Skip to content

Comments

cctag: init at 1.0.3#216402

Merged
davidak merged 1 commit intoNixOS:masterfrom
hesiod:cctag
Mar 30, 2023
Merged

cctag: init at 1.0.3#216402
davidak merged 1 commit intoNixOS:masterfrom
hesiod:cctag

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
Draft
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 a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 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
@ofborg ofborg bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Mar 16, 2023
@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

davidak
davidak reviewed Mar 18, 2023
View reviewed changes
davidak
davidak approved these changes Mar 18, 2023
View reviewed changes
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

davidak
davidak reviewed Mar 18, 2023
View reviewed changes
SuperSandro2000
SuperSandro2000 reviewed Mar 18, 2023
View reviewed changes
pkgs/development/libraries/cctag/default.nix Outdated
Copy link
Member

@SuperSandro2000 SuperSandro2000 Mar 18, 2023

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

@hesiod hesiod Mar 26, 2023

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"

SuperSandro2000
SuperSandro2000 reviewed Mar 18, 2023
View reviewed changes
@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

Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

+1 more reviewer

@davidak davidak davidak approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hesiod @davidak @SuperSandro2000