SWIFTSIM / SWIFT

Modern astrophysics and cosmology particle-based code. Mirror of gitlab developments at https://gitlab.cosma.dur.ac.uk/swift/swiftsim
http://www.swiftsim.com
GNU Lesser General Public License v3.0
88 stars 58 forks source link

Onboarding guide could suggest better configure flags #45

Open apontzen opened 8 months ago

apontzen commented 8 months ago

Overall the onboarding guide is very helpful. However, I found I had to specify paths to FFTW3 and GSL, even though these were both accessible via pkg-config. Moreover despite these being listed as essential requirements, the configure process did not complain but rather allowed me to compile a binary that was incapable of running the cosmological example.

This was easily fixed but my onboarding experience could have been even smoother, and for inexperienced users it could be particularly useful to directly instruct them on the flags.

(In case relevant, this is on MacOS with macports-provided gcc/pkg-config/fftw3/gsl)

MatthieuSchaller commented 8 months ago

Thanks for the suggestion/feedback.

Is that the standard way packages are installed on OSX? If so, indeed, it looks like we would need to provide more info as direct detection seems to not be working.

apontzen commented 8 months ago

There are two main package managers for MacOS, macports and homebrew, both in common usage. There is also fink but I think that is less well used.

It is possible there is some quirk in my machine's configuration that made it harder for automake to track down the installed packages (in honesty I don't understand the innards of automake). But I haven't done anything unusual.

It was very easily fixed by adding --with-gsl=/opt/local/ --with-fftw=/opt/local/. Homebrew's prefix is /usr/local/ I think.

MatthieuSchaller commented 8 months ago

Ok, great. Then indeed we should squeeze this into the two pages.

pwdraper commented 8 months ago

The less verbose way to find packages from an installation tree outside of the system directories would be to use something like:

configure CFLAGS="-I/opt/local/include" LDFLAGS="-L/opt/local/lib"

Or even better:

configure CFLAGS="-I/opt/local/include" LDFLAGS="-L/opt/local/lib -Wl,-rpath,/opt/local/lib"

which on Linux would avoid needing to also define LD_LIBRARY_PATH (DYLD_LIBRARY_PATH on os x I think).

pwdraper commented 8 months ago

The paths used in the --with options are really for pointing at one-off builds rather than a whole other installation tree.

MatthieuSchaller commented 7 months ago

Isn't the option you suggest more complex for a user not familiar with these things?

pwdraper commented 7 months ago

Not really, it is the easiest way to pull in a number of packages installed into a third party tree. Ideally you'd add that tree to the ld.so mechanisms and compiler default search directories, but that seems to be an issue for OS X.

Note you can set these as environment variables and just run ./configure, so good things for init scripts (this is how all the modules on COSMA work). Suspect that is the way I would do it. Anyway, feel free to update as you like anything is better than nothing.

apontzen commented 7 months ago

It didn't work for me to pass these in as environment variables -- I actually had to pass it to --with-fftw etc. This did surprise me. It's possible of course something was strange in what I was doing - this is now a month ago or so, so I'd need to do some work if you want to know whether it's reproducible.

pwdraper commented 7 months ago

Thanks, it should work, although maybe I should have used CPPFLAGS instead of CFLAGS, but that usually doesn't make a difference. For interest if you could try it once more (with CPPFLAGS) and attach the config.log file it produces, or have a look yourself, that should report why the check for the library failed.

WillJRoper commented 4 months ago

Late to the party but just to add a few things:

MatthieuSchaller commented 4 months ago

Makes me realise I forgot to make these changes... I'll try to add something about --with-xxx=... whilst remaining under the page limit.

MatthieuSchaller commented 4 months ago

What about this version: onboarding.pdf

I have removed the part of the docs about doxygen to gain space. That's not really an on-boarding thing.

WillJRoper commented 4 months ago

Looks good to me. My only suggestion would be to include the fact that for HDF5 you need to point directly at h5cc rather than the directory but you're fairly pushed for space.

apontzen commented 4 months ago

Looks good to me. Strangely I found myself recompiling swift just a couple of days ago, and ran into a new problem. Specifically, when compiling, I received

In file included from random.h:33,
                 from power_spectrum.c:43:
sincos.h:43:51: error: declaration of 'sincos' shadows a built-in function [-Wshadow]
   43 | __attribute__((always_inline)) INLINE static void sincos(const double x,
      |                                                   ^~~~~~
sincos.h:65:51: error: declaration of 'sincosf' shadows a built-in function [-Wshadow]
   65 | __attribute__((always_inline)) INLINE static void sincosf(const float x,
      |                                                   ^~~~~~~

I was able to compile by removing -Werror from line 1842 of configure.ac. This demoted these errors back to warnings. But in honesty, I couldn't really see why the warnings were arising anyway - this is with gcc, so sincos etc ought to be defined. It might be something to do with macOS and its built-in preference for clang over gcc. But I didn't have time to dig further.

It might not be possible to do anything with this information, but I mention it just in case helpful.

MatthieuSchaller commented 4 months ago

Thanks both.

In this case, --enable-compiler-warnings is the trick to not edit the Makefile directly.

We have seen this sincos issue on OSX before. We may not be detecting something correctly. Could it be that clang has eventually implemented sincos and we don't need to provide an alternative?

apontzen commented 4 months ago

I wouldn't guess --enable-compiler-warnings has the effect of disabling errors.

I would imagine the issue is something to do with MacOS's preference for clang over gcc, but this was actually being compiled with gcc. Perhaps it was somehow finding the wrong math.h header.

MatthieuSchaller commented 4 months ago

It actually allows warnings by not turning them automatically into errors (which is our default).

I think there is no standard convention for the name of the autotool flag enabling/disabling -Werror. We could maybe add an alias like --disable-werror?

MatthieuSchaller commented 4 months ago

Regarding sincos, it's technically a GCC extension of the C standard. We provide our own functions in the case where we detected that we are compiling on a system where the function does not exist. It looks like in your case, the configuration seems to think sincos is not present and thus enables our version which then conflicts with the system's one.

apontzen commented 4 months ago

yes, that’s definitely what’s happening — just not sure why!

MatthieuSchaller commented 4 months ago

Could you link here the config.log and config.h files that were generated?