Fix gdal installation errors on NOAA RDHPC Gaea (64bit I/O) and invalid json-c library dir#30809
Fix gdal installation errors on NOAA RDHPC Gaea (64bit I/O) and invalid json-c library dir#30809climbfuji wants to merge 4 commits intospack:developfrom
Conversation
| ] | ||
| # Fix for gdal always using suffix 'lib', even though libraries | ||
| # like json-c might get installed in 'lib64' | ||
| #ldflags.append(self.spec['libtiff'].libs.search_flags) |
There was a problem hiding this comment.
Should we do the same for libtiff and libgeotiff?
There was a problem hiding this comment.
I'm surprised that this solution is needed. Doesn't Spack automatically inject these into the compiler search path already?
There was a problem hiding this comment.
Apparently not. I wasn't the only one having the issue, when I wanted to create one for this error I found that @tldahlgren had the same problem.
There was a problem hiding this comment.
@scheibelp @becker33 why would this not be done automatically by our compiler wrappers and build_environment.py?
There was a problem hiding this comment.
Yes it should be done automatically. I'd be interested in seeing the compiler wrapper log generated when installing this with spack -d.... This (error and the fix) could happen if the build bypasses the Spack compiler wrappers (in which case the wrapper never gets a chance to insert the -L, but setting LDFLAGS in configure works because the build system then arranges things properly); in that case, the compiler wrapper log will be boring (but it will be apparent that the log is missing an entry for whatever compilation line fails in the build log).
There was a problem hiding this comment.
I think OSGeo/gdal#3895 (comment) has the logs you want (note that it's ~1 yr old)
There was a problem hiding this comment.
Apologies I did not make this clear but I would also need the spack-build-out.txt log to see which file failed, at which point I could cross-reference with the compiler wrapper log. Sometimes that is just the last entry in the log, but when I opened the log you posted there, the last entry included a -L<json-prefix>/lib64, so that looked good.
Also, I double checked our default library-finding logic and it appears to be capable of checking both lib and lib64.
There was a problem hiding this comment.
Scroll up to the top of that same issue for the build log (spack-build-out.txt).
There was a problem hiding this comment.
Interesting, the line which fails in the build log:
/bin/sh /data/geol552/a/adamjs5/spack-stage-gdal-3.2.3-ozkzmsnz4b4ucfeag6ezpb7xg6e67kpf/spack-src/libtool --mode=link --silent /data/keeling/a/adamjs5/spack/lib/spack/env/gcc/g++ gdalinfo_bin.lo /data/geol552/a/adamjs5/spack-stage-gdal-3.2.3-ozkzmsnz4b4ucfeag6ezpb7xg6e67kpf/spack-src/libgdal.la -o gdalinfo
Has no corresponding wrapper invocation, which suggests to me that libtool --mode=link is bypassing the Spack compiler wrapper (even though you refer to /data/keeling/a/adamjs5/spack/lib/spack/env/gcc/g++) or doing some check and deciding things won't work before it invokes it.
|
@adamjstewart Will fix the style errors once we agree on the variant name and what to do with libtiff and libgeotiff. |
|
My only concern with this variant is that I don't see any equivalent in the new CMake build system (added in 3.5.0). Of course, we could make this variant available only in 3.4 and older, but I want to make sure that you don't have the same issue when I added 3.5.0. |
I can try building 3.5.0 on Gaea (our spack fork is a bit behind, don't have this version yet I think). |
|
I haven't added it to the package yet because it switches from Autotools to CMake. I'm planning on working on this shortly after Spack 0.18 is released (in the next week). |
Actually, I don't see 3.5.0 yet in the repo: https://github.com/spack/spack/blob/develop/var/spack/repos/builtin/packages/gdal/package.py |
I see. How to proceed? |
|
My personal preference would be to wait until I finish the CMake refactor (probably 2 week timeframe). If the new version that uses CMake fixes all of our issues, then great! If people still want to be able to install the older versions that require these hacks, then we can still merge them for the older Autotools-only versions. Does that work for you, or would you like to see this fixed sooner than 2 weeks? |
I need a fix sooner than that, but in our own fork. I can update our fork with changes that go on top of this PR, but it would be good to choose the right name for the 64bitIO variant (because if the uppercase letters) for now. |
adamjstewart
left a comment
adamjstewart
left a comment
There was a problem hiding this comment.
+unix_stdio_64 changes look good and I'm happy to merge those right away.
For the ldflags changes, I want to better understand why these are needed before merging. These should be added automatically by the compiler wrappers. If they aren't, we can either fix that, or manually add them like you do here for all deps, not just json-c.
Ok, thanks. I'll merge the commits in this PR in our fork and add any follow-up changes we are making here later. What is in the PR right now is sufficient to build on Gaea. Thanks for your quick feedback! |
|
Also, if you figure out a way to auto-detect whether or not |
|
@adamjstewart As it turns out the new variant My assumption is that What's the best way to deal with this? Simply make the default for variant |
|
Interesting. We could set it up so that |
|
Now that Spack 0.18 is out I'll be working on porting the package to CMake for the newest GDAL release. Hopefully that solves your issues because it seems like the Autotools build is wonky on your system. |
3 participants
Fixes #23943 by using the correct library directory name (
liborlib64, depending on the system) when looking forjson-clibraries.Fixes #30808 by adding a new variant
64bitIO. Default is on, i.e. no change, turning it off on Gaea allows building[email protected]successfully.