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

Jump to bottom

cloudcompare: 2.11.0 → 2.11.2 #101879

Merged
merged 1 commit into from Oct 28, 2020
Merged

cloudcompare: 2.11.0 → 2.11.2 #101879

merged 1 commit into from Oct 28, 2020

Conversation

sikmir
Copy link
Member

@sikmir sikmir commented Oct 27, 2020

Motivation for this change
  • Changelog
  • Replace -DCOMPILE_CC_CORE_LIB_WITH_TBB=ON with -DCCCORELIB_USE_TBB=ON
  • Remove outdated comments
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@sikmir sikmir requested a review from nh2 October 27, 2020 21:23
@nh2
Copy link
Contributor

nh2 commented Oct 28, 2020

Result of nixpkgs-review pr 101879 1

1 package failed to build:
  • cloudcompare

@nh2
Copy link
Contributor

nh2 commented Oct 28, 2020

Result of nixpkgs-review pr 101879 1

1 package built:
  • cloudcompare

nh2
nh2 requested changes Oct 28, 2020
View reviewed changes
Copy link
Contributor

@nh2 nh2 left a comment •
edited

Choose a reason for hiding this comment

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

I was wrong with writing in > 2.11.0 in the code when writing the previous version of the derivation; even though those changes were in CloudCompare's master, they didn't go into 2.11 and I'm not sure they will go into 2.12 either.

Thus the build emits:

CMake Warning at CC/CMakeLists.txt:33 (message):
  CCLib configured without parallel algorithm capabilities - see
  COMPILE_CC_CORE_LIB_WITH_TBB

Also the removed

-    # As of writing includes (https://github.com/CloudCompare/CloudCompare/blob/a1c589c006fc325e8b560c77340809b9c7e7247a/.gitmodules):
-    # * libE57Format
-    # * PoissonRecon

is still true, and the submodules are even growing, so we should keep that comment.

I'll fix these two things myself in a moment.

@sikmir
Copy link
Member Author

sikmir commented Oct 28, 2020

is still true, and the submodules are even growing, so we should keep that comment.

I just wonder, what is the reason to keep comment with list of submodules? It's easy to obtain from upstream repo, but keeping comment up to date is additional effort.

@ofborg ofborg bot requested a review from nh2 October 28, 2020 12:38
@nh2
Copy link
Contributor

nh2 commented Oct 28, 2020 •
edited

I just wonder, what is the reason to keep comment with list of submodules?

@sikmir Two main reasons:

  • To make clear that / why the derivation doesn't depend on e.g. libE57Format from nixpkgs. It's a common issue in nixpkgs to be surprised that some tool didn't get e.g. a security fix even though the corresponding package was fixed. That's why it's useful to point out explicitly that the package vendors a dependency that we also have in nixpkgs.
  • To explain why the package may have more dependencies than stated in its README.

It's true that one can conclude it from the upstream repo, but fetchSubmodules can easily go unnoticed to a drive-by contributor that may not follow indirections, so making it visible at first look in the derivation can avoid additional work.

@nh2
Copy link
Contributor

nh2 commented Oct 28, 2020

Runs well, merging, thanks for the update!

@nh2 nh2 merged commit c06f86d into NixOS:master Oct 28, 2020
@sikmir
Copy link
Member Author

sikmir commented Oct 28, 2020

@sikmir Two main reasons:

  • To make clear that / why the derivation doesn't depend on e.g. libE57Format from nixpkgs. It's a common issue in nixpkgs to be surprised that some tool didn't get e.g. a security fix even though the corresponding package was fixed. That's why it's useful to point out explicitly that the package vendors a dependency that we also have in nixpkgs.
  • To explain why the package may have more dependencies than stated in its README.

It's true that one can conclude it from the upstream repo, but fetchSubmodules can easily go unnoticed to a drive-by contributor that may not follow indirections, so making it visible at first look in the derivation can avoid additional work.

Thanks, I will keep it in mind now.

@sikmir sikmir deleted the cloudcompare branch October 28, 2020 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nh2 nh2 Awaiting requested review from nh2

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sikmir @nh2