Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

astroterm: init at 1.0.4, maintainers: add da-luce #373316

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

da-luce
Copy link
Contributor

@da-luce da-luce commented Jan 12, 2025

Adds a new package for da-luce/astroterm. This is a celestial viewer for the terminal, displaying stars, planets, and more. For example:

image

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Jan 12, 2025
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jan 12, 2025
@SigmaSquadron SigmaSquadron added the 11.by: upstream-developer This issue or PR was created by the developer of packaged software label Jan 13, 2025
Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome to Nixpkgs! Thanks for packaging your software.

pkgs/by-name/as/astroterm/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/as/astroterm/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/as/astroterm/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/as/astroterm/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/as/astroterm/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/as/astroterm/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/as/astroterm/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/as/astroterm/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/as/astroterm/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/as/astroterm/package.nix Outdated Show resolved Hide resolved
@SigmaSquadron SigmaSquadron added the 8.has: package (new) This PR adds a new package label Jan 13, 2025
@da-luce da-luce force-pushed the init-astroterm branch 2 times, most recently from e3b6215 to c6fe6cb Compare January 13, 2025 02:26
@da-luce da-luce requested a review from SigmaSquadron January 13, 2025 03:22
Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nixpkgs-review is being weird due to the OfBorg failure, but that can be ignored.

Built on x86_64-linux:

$ ./astroterm --version
astroterm 1.0.3

@da-luce
Copy link
Contributor Author

da-luce commented Jan 13, 2025

nixpkgs-review is being weird due to the OfBorg failure, but that can be ignored.

@SigmaSquadron, could you clarify why it's safe to ignore this failure? My initial suspicion is that xxd might not have generated a valid header due to some unexpected behavior on the Linux build. Since I'm not very familiar with OfBorg or Nix, could you provide some guidance on how to begin debugging this?

@SigmaSquadron
Copy link
Contributor

The majority of our CI system is currently broken, and OfBorg shouldn't even be building things. This error can be ignored because the build works on an actual Linux system, and OfBorg results cannot be trusted until its builders are restored to a working state.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 14, 2025
@FliegendeWurst
Copy link
Member

I get a build failure when building on master: https://paste.fliegendewurst.eu/LyBtYd.log

FAILED: libastroterm_lib.a.p/src_core.c.o 
gcc -Ilibastroterm_lib.a.p -I. -I.. -I../include -Idata -I../data -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Wpedantic -DHAVE_ARGTABLE3 -fPIC -MD -MQ libastroterm_lib.a.p/src_core.c.o -MF libastroterm_lib.a.p/src_core.c.o.d -o libastroterm_lib.a.p/src_core.c.o -c ../src/core.c
../src/core.c: In function 'string_to_time':
../src/core.c:493:27: error: implicit declaration of function 'strptime'; did you mean 'strftime'? [-Wimplicit-function-declaration]
  493 |     const char *pointer = strptime(string, "%Y-%m-%dT%H:%M:%S", time);
      |                           ^~~~~~~~
      |                           strftime
../src/core.c:493:27: error: initialization of 'const char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]

@da-luce da-luce changed the title astroterm: init at 1.0.3, maintainers: add da-luce astroterm: init at 1.0.4, maintainers: add da-luce Jan 14, 2025
@FliegendeWurst
Copy link
Member

Build log says Checking for function "strptime" : YES and still fails with the same error.

@da-luce
Copy link
Contributor Author

da-luce commented Jan 14, 2025

@FliegendeWurst I suspect the local implementation not be including the header correctly. Will update once fixed.

I would test locally but I'm unable to reproduce on WSL. Windows uses the local implementation but MSVC doesn't complain about implicit declarations.

Update: issue may have been that certain definitions were missing as described here

@FliegendeWurst
Copy link
Member

Do you set -no-pie somewhere in your build? We are planning to enable PIE by default soon.

@da-luce
Copy link
Contributor Author

da-luce commented Jan 14, 2025

Do you set -no-pie somewhere in your build? We are planning to enable PIE by default soon.

Yeah. The argtable3 library on Homebrew for Linux is non-PIC, so the only fix I found for that was to make my own code non-PIC.

@FliegendeWurst
Copy link
Member

Our argtable is PIC. So it would be good make -no-pie optional :)

@da-luce
Copy link
Contributor Author

da-luce commented Jan 14, 2025

Good point. Should have been passing as a CLI arg and not baked into the build file in the first place. Removed -no-pie flag.

@sikmir
Copy link
Member

sikmir commented Jan 15, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 373316


x86_64-darwin

✅ 1 package built:
  • astroterm

@da-luce da-luce requested a review from sikmir January 15, 2025 18:56
@sikmir sikmir added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Jan 15, 2025
@sikmir
Copy link
Member

sikmir commented Jan 15, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 373316


x86_64-linux

✅ 1 package built:
  • astroterm

@sikmir sikmir merged commit 2b6ea7d into NixOS:master Jan 15, 2025
25 of 29 checks passed
@da-luce da-luce deleted the init-astroterm branch January 15, 2025 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: upstream-developer This issue or PR was created by the developer of packaged software 12.approvals: 2 This PR was reviewed and approved by two reputable people 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants