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.

Sign up for GitHub

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

Jump to bottom

geogram: init at 1.8.3 #215728

Merged
merged 1 commit into from
Mar 18, 2023
Merged

geogram: init at 1.8.3 #215728

merged 1 commit into from
Mar 18, 2023

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)
@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 10.rebuild-darwin: 1-10 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 requested 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.

pkgs/development/libraries/geogram/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/geogram/default.nix Show resolved Hide resolved
pkgs/development/libraries/geogram/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/geogram/default.nix Outdated Show resolved Hide resolved
@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 10.rebuild-darwin: 1 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 requested 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

pkgs/development/libraries/geogram/default.nix Show resolved Hide resolved
pkgs/development/libraries/geogram/default.nix Show resolved Hide resolved
pkgs/development/libraries/geogram/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/geogram/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/geogram/default.nix Show resolved Hide resolved
pkgs/development/libraries/geogram/default.nix Outdated Show resolved Hide resolved
@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

@davidak davidak davidak approved these changes

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hesiod @davidak