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

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.
@nh2
Copy link
Contributor

@nh2 nh2 commented Oct 28, 2020

Result of nixpkgs-review pr 101879 1

1 package failed to build:
  • cloudcompare

@nh2
Copy link
Contributor

@nh2 nh2 commented Oct 28, 2020

Result of nixpkgs-review pr 101879 1

1 package built:
  • cloudcompare

Copy link
Contributor

@nh2 nh2 left a comment

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.

@nh2 nh2 force-pushed the cloudcompare branch from 9323d86 to 59645d2 Oct 28, 2020
@sikmir
Copy link
Member Author

@sikmir 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 Oct 28, 2020
@nh2
Copy link
Contributor

@nh2 nh2 commented Oct 28, 2020

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 nh2 commented Oct 28, 2020

Runs well, merging, thanks for the update!

@nh2 nh2 merged commit c06f86d into NixOS:master Oct 28, 2020
17 of 19 checks passed
@sikmir
Copy link
Member Author

@sikmir 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 Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants