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

suitesparse: 5.4.0 → 5.7.1 + clean up #82274

Merged
merged 7 commits into from
Mar 22, 2020
Merged

suitesparse: 5.4.0 → 5.7.1 + clean up #82274

merged 7 commits into from
Mar 22, 2020

Conversation

jtojnar
Copy link
Member

@jtojnar jtojnar commented Mar 10, 2020 •
edited
Loading

Motivation for this change

Also had split (e.g. GEGL depends only on umfpack) in the works but it does not make sense to delay it.

cc @knedlsepp @ttuegel @jluttine

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.

@jtojnar
suitesparse: clean up
40ae287 
* Split to multiple outputs
* Fetch source from GitLab (the only source for future releases)
* Re-order attrset to be more idiomatic
* Format with nixpkgs-fmt
@jtojnar
mongoose: init at 2.0.4
b4e6293
@ofborg ofborg bot added 6.topic: python 8.has: package (new) This PR adds a new package labels Mar 10, 2020
@ofborg ofborg bot requested review from edwtjo and ttuegel March 10, 2020 19:49
@jtojnar
Copy link
Member Author

jtojnar commented Mar 10, 2020

Upstream also now suggests to link against Intel MKL instead of OpenBLAS but it is just something they discovered recently, not a regression. As such, I will not touch that until we know more.

knedlsepp
knedlsepp approved these changes Mar 10, 2020
View reviewed changes
Copy link
Member

@knedlsepp knedlsepp left a comment

Choose a reason for hiding this comment

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

Thanks for giving this a good cleanse.

@knedlsepp
Copy link
Member

Interesting findings about MKL/OpenBLAS. I guess we probably care about the licensing quite a bit so the swap wouldn't be all that straightforward anyway.

@knedlsepp
Copy link
Member

Was graphblas part of suitesparse before? I remember seeing quite a few log messages as part of the build.

@jtojnar
Copy link
Member Author

jtojnar commented Mar 10, 2020

Yes it was. It is also what made the build take so long.

@jtojnar
suitesparse: do not build Mongoose & GraphBLAS
c16d5b6 
We already have package for them
@jtojnar
suitesparse: ultimate clean-up
b552b84 
* Switch to default buildPhase & installPhase
* In preConfigure
	* Do not add -DNPARTITION to CHOLMOD_CONFIG. That would disable the use of Metis but we already have that.
	* Do not remove -lrt on Darwin, Darwin compiler can handle that and the code no longer exists anyway.
	* With CUDA enabled
		* Do not replace CUDA_ROOT. It does not exist any more. Instead we are setting CUDA_PATH in makeFlags.
		* Do not replace GPU_BLAS_PATH, it defaults to CUDA_PATH so it will end up with the same value.
		* Do not add -DCHOLMOD_OMP_NUM_THREADS to GPU_CONFIG. Why would be having the library use the same number of threads as the builder a good idea?
		* Do not replace CUDA_PATH, we are setting it in makeFlags now.
		* Do not replace CUDART_LIB and CUBLAS_LIB. They were being replaced incorrectly (cuda libs are located in lib directory, not lib64). Instead set the correct paths in makeFlags.
		* Do not replace CUDA_INC_PATH. Its default looks like it will end up with the same value.
		* Do not replace NV20, NV30, NV35 – not used any more.
		* Do not replace NVCC, defaults to the same.
		* Do not replace NVCCFLAGS, we just used the default from SourceSparse 4.4.7 with -gencode=arch=compute_60,code=compute_60 tacked on top. Current upstream default looks much better.
* Stop adding -DNTIMER to CFLAGS on Darwin – clock_gettime is supported by macOS 10.12 SDK.
* In buildPhase
	* Move the make arguments to makeFlags and library to buildFlags, allowing us to drop the manual make call. I did not verify all of these are still needed.
	* Remove the creation of libsuitesparse.so. As far as I could tell it is some kind of remnant of our old expression – perhaps due to past deficiencies of the build scripts, we created the individual libraries as symlinks to libsuitesparse.so: NixOS@e36b3ec But since the build script can now build individual .so libraries, there should be no need for this abomination. No other distros do this either.
* In installPhase
	* No need to copy things manually, there is an install target. We just need to pass INSTALL=$out flag to make to let it know where to install the files.
	* I do not have means of verifying the darwin dylib name fix but it looks like it might be fixed in an upcoming release.
	* I dropped the rpath fixup as it does not seem to be needed any more (ldd does not report any unresolved libraries).
@jtojnar
Copy link
Member Author

jtojnar commented Mar 11, 2020 •
edited
Loading

I have spent a couple hours and purged it completely (see the commit message).

It builds both with and without cuda.

Now only checking the makeFlags if they are still needed remains but I have spent too much time on this already.

In the future we might clean the expression even further:

jonringer
jonringer reviewed Mar 13, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

imo, the suitesparse commits should be squashed.

falls under the small fixup clause of if you have commits pkg-name: oh, forgot to insert whitespace: squash commits in this case. https://nixos.org/nixpkgs/manual/#submitting-changes-making-patches

@jtojnar
Copy link
Member Author

jtojnar commented Mar 13, 2020 •
edited
Loading

IMHO it make sense to have structural and functional changes in separate commits, not only for reviewing but also for making sense of it when blaming files. Otherwise the structural changes will overshadow the functional ones.

@jonringer
Copy link
Contributor

there is a lot going on, and I don't have enough context on how these applications are meant to behave, to have much of an opinion.

the diff LGTM, so I'll defer to your preference.

@jtojnar
Copy link
Member Author

jtojnar commented Mar 13, 2020 •
edited
Loading

I would like to hear from @ttuegel who introduced the combined libsuitesparse.so if they still have some need for it. If there is no response, we should merge it after a week.

@jtojnar jtojnar merged commit d951c53 into NixOS:master Mar 22, 2020
@jtojnar jtojnar deleted the suitesparese-5.6 branch March 22, 2020 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer jonringer approved these changes

@knedlsepp knedlsepp knedlsepp approved these changes

@edwtjo edwtjo edwtjo approved these changes

@FRidh FRidh Awaiting requested review from FRidh

@ttuegel ttuegel Awaiting requested review from ttuegel

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jtojnar @knedlsepp @jonringer @edwtjo