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

cctag: init at 1.0.3 #216402

Merged
merged 1 commit into from
Mar 30, 2023
Merged

cctag: init at 1.0.3 #216402

merged 1 commit into from
Mar 30, 2023

Conversation

hesiod
Copy link
Contributor

@hesiod hesiod commented Feb 15, 2023

Description of changes

Add package for the CCTag library.

Links:

Depends on PR #215689

Part of an ongoing effort to package Meshroom (#94127). See also #215528 #215728 #216401 #216399

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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.

@hesiod hesiod mentioned this pull request Feb 15, 2023
19 tasks
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Feb 15, 2023
@ofborg ofborg bot requested a review from thoughtpolice February 15, 2023 00:43
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Feb 15, 2023
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 17, 2023
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 9, 2023
@hesiod
Copy link
Contributor Author

hesiod commented Mar 9, 2023

Waiting for #217585 to land in master

@hesiod hesiod marked this pull request as ready for review March 16, 2023 14:58
@hesiod hesiod requested a review from davidak March 16, 2023 14:59
@hesiod
Copy link
Contributor Author

hesiod commented Mar 16, 2023

Result of nixpkgs-review pr 216402 run on x86_64-linux 1

1 package built:
  • cctag

@davidak
Copy link
Member

davidak commented Mar 18, 2023

Result of nixpkgs-review pr 216402 run on x86_64-linux 1

3 packages built:
  • cctag
  • cctag.dev
  • cctag.lib

Copy link
Member

@davidak davidak left a comment

Choose a reason for hiding this comment

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

Code looks good

Builds, can't test it

pname = "cctag";
version = "1.0.3";

outputs = [ "lib" "dev" "out" ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
outputs = [ "lib" "dev" "out" ];
outputs = [ "out" "lib" "dev" ];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that really a good idea? cctag.out is empty (it's only included because some hooks complain otherwise). cctag.lib really is the main binary output.
If cctag.lib were to go first, building cctag would default to an empty output which would be somewhat counterintuitive.
Not sure if there are any policies surrounding this other than "binaries should go first"

@hesiod
Copy link
Contributor Author

hesiod commented Mar 28, 2023

@davidak I'm reasonably confident that CI should run fine on Darwin now (let's see in a few minutes).
Please have a look at whether my use of doCheck (disabled on Darwin) in deciding whether to build the tests (in cmakeFlags) is OK. I can't see why not, but I haven't seen similar logic in Nixpkgs yet. But perhaps that's because compile issues that only affect the tests might be rare.

@davidak davidak merged commit 93feafa into NixOS:master Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants