Skip to content

Comments

geogram: init at 1.8.3#215728

Merged
davidak merged 1 commit intoNixOS:masterfrom
hesiod:geogram
Mar 18, 2023
Merged

geogram: init at 1.8.3#215728
davidak merged 1 commit intoNixOS:masterfrom
hesiod:geogram

Conversation

@hesiod
Copy link
Contributor

@hesiod hesiod commented Feb 10, 2023 •
edited by davidak
Loading

Description of changes

Add the Geogram library.
Part of an ongoing effort to package Meshroom (#94127).

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 10, 2023
Merged
12 tasks
hesiod added a commit to hesiod/nixpkgs that referenced this pull request Feb 10, 2023
@hesiod
pyside2: Add qt3d to buildInputs
53cd64c 
PySide2 will not build its qt3d component unless qt3d is added to
buildInputs.
This is part of the effort to package Meshroom.
(Related Meshroom PRs: NixOS#215528 and NixOS#215728)
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 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-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. labels Feb 10, 2023
@hesiod hesiod force-pushed the geogram branch 2 times, most recently from 22b9ef1 to 9edb082 Compare February 10, 2023 22:24
@hesiod hesiod marked this pull request as ready for review February 10, 2023 22:24
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. and removed 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. labels Feb 10, 2023
@davidak
Copy link
Member

davidak commented Feb 14, 2023

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

1 package built:
  • geogram

davidak
davidak suggested changes Feb 14, 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.

Builds successful. The result has only 2 files. Is that expected?

[nix-shell:~/.cache/nixpkgs-review/pr-215728]$ tree results/
results/
└── geogram -> /nix/store/j64i56kvgg5wp5m1snhxwmm9wcbg2zm1-geogram-1.8.2-bin

1 directory, 0 files

[nix-shell:~/.cache/nixpkgs-review/pr-215728]$ tree results/geogram/
results/geogram/
└── bin
    ├── geodump
    └── vorpalite

1 directory, 2 files

The binaries start without error.

Not sure how to review the other outputs.

Generally looks good. Please see the other comments.

@hesiod
Copy link
Contributor Author

hesiod commented Feb 14, 2023

Builds successful. The result has only 2 files. Is that expected?

It's a multiple output package, the interesting part is in the lib output. The binaries aren't that important.

@hesiod hesiod mentioned this pull request Feb 14, 2023
Merged
12 tasks
@davidak davidak mentioned this pull request Feb 15, 2023
Closed
@hesiod hesiod mentioned this pull request Feb 15, 2023
Draft
12 tasks
@davidak
Copy link
Member

davidak commented Feb 15, 2023

Library is successfully built:

[nix-shell:~/.cache/nixpkgs-review/pr-215728-4]$ tree result-lib
result-lib
└── lib
    ├── libgeogram.so -> libgeogram.so.1
    ├── libgeogram.so.1 -> libgeogram.so.1.8.2
    └── libgeogram.so.1.8.2

This was referenced Feb 15, 2023
Merged
Draft
@figsoda figsoda mentioned this pull request Feb 15, 2023
Merged
@hesiod hesiod marked this pull request as draft March 9, 2023 20:58
@hesiod hesiod force-pushed the geogram branch 2 times, most recently from 78bf4ed to c121790 Compare March 9, 2023 21:00
@hesiod
Copy link
Contributor Author

hesiod commented Mar 9, 2023

Amended with the following changes:

  • Add documentation (doc output)
  • Improve multi output install rules (use patch instead of postInstall)
  • Replace bundled zlib
  • Update to 1.8.3 (previous version was 1.8.2)
  • Add some comments regarding cmakeFlags entries

@hesiod hesiod marked this pull request as ready for review March 9, 2023 21:02
@ofborg ofborg bot added 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. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. labels Mar 9, 2023
@hesiod
Copy link
Contributor Author

hesiod commented Mar 9, 2023

Marked as broken on aarch64-linux (some upstream bug)
@davidak I addressed all your comments, the PR should be ready for another review (if you have the time)

@hesiod
Copy link
Contributor Author

hesiod commented Mar 9, 2023

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

1 package built:
  • geogram

@davidak
Copy link
Member

davidak commented Mar 10, 2023

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

5 packages built:
  • geogram
  • geogram.bin
  • geogram.dev
  • geogram.doc
  • geogram.lib

davidak
davidak suggested changes Mar 10, 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.

generally looks good

check my comments

@hesiod hesiod mentioned this pull request Mar 12, 2023
Closed
@hesiod hesiod marked this pull request as draft March 14, 2023 15:13
@hesiod
Copy link
Contributor Author

hesiod commented Mar 14, 2023

Currently working on a few improvements to the checkPhase so I marked the PR as a draft, other than that the PR is in a good shape already

@hesiod hesiod changed the title geogram: init at 1.8.2 geogram: init at 1.8.3 Mar 14, 2023
@hesiod
Copy link
Contributor Author

hesiod commented Mar 14, 2023

I've enabled the tests again, although I had to disable a few failing tests. I've also improved the checkPhase a bit (previously it would not actually abort the build if some tests failed)

@hesiod hesiod marked this pull request as ready for review March 14, 2023 15:54
@hesiod hesiod requested a review from davidak March 14, 2023 15:55
@davidak
Copy link
Member

davidak commented Mar 18, 2023

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

5 packages built:
  • geogram
  • geogram.bin
  • geogram.dev
  • geogram.doc
  • geogram.lib

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, binaries run

@davidak davidak merged commit 64b8d10 into NixOS:master Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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.

2 participants

@hesiod @davidak