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

Fix --enabled-shared for configure #1984

Merged
merged 13 commits into from
Apr 3, 2020
Merged

Fix --enabled-shared for configure #1984

merged 13 commits into from
Apr 3, 2020

Conversation

dschwoerer
Copy link
Contributor

Fix #1126

Switch all builds to shared object. That decreases the build time. (roughly 5 mins for each build, total ~30 min)

Disabling debug infos also should help decrease the sizes, and thus build time. However, not as strong of an effect as I hoped.

Automatically set the target to the shared object, if we are building
the shared object.
This removes the need to call `make shared` after configuring with
`--enable-shared`.
@ZedThree
Copy link
Member

Very nice, thanks @dschwoerer ! 🎉

It's might also be possible to use the gold linker to speed things up further. I did attempt this awhile ago: https://github.com/boutproject/BOUT-dev/tree/use-gold-travis

We probably want at least one Travis build to build static instead of shared. There can be issues of differences in symbol visibility between static/shared, so it would be good to try and catch these if possible.

configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
@dschwoerer
Copy link
Contributor Author

Any preferences on which job should be static?

@ZedThree
Copy link
Member

Ideally we'd have it be a separate job, but that's probably a waste of resources!

Let's make the GCC7 job static, for lack of an obvious choice.

Also, does -g0 have much effect on build times? I guess it could be important on Travis where the filesystem is not so great

@dschwoerer
Copy link
Contributor Author

I don't think -g0 is important. I think the draw-back of not-so-nice backtraces on travis isn't that important?
That should basically reduce the library size to it's stripped size, roughly 10 MB rather then 500 MB.
Probably fine to keep it only in the static case, and revert in the shared case, as we only write once then ...

@ZedThree
Copy link
Member

Yeah, missing backtraces don't matter so much, as I'll likely just run it locally in gdb anyway.

Probably fine to keep it only in the static case, and revert in the shared case, as we only write once then

Sounds reasonable!

@ZedThree
Copy link
Member

ZedThree commented Apr 2, 2020

Coverage tests are failing:

/usr/bin/ld: test_multigrid_laplace: hidden symbol `__gcov_init' in /usr/lib/gcc/x86_64-linux-gnu/5/libgcov.a(_gcov.o) is referenced by DSO
/usr/bin/ld: final link failed: Bad value

Push tests don't run the coverage jobs which is why they pass.

I'm a bit confused because we definitely have --coverage in both BOUT_FLAGS and LDFLAGS which should be enough. I'll have a quick investigate.

src/makefile.in Outdated Show resolved Hide resolved
@ZedThree
Copy link
Member

ZedThree commented Apr 2, 2020

Need $(LDFLAGS) adding when making the .so, and might need the following change as well:

modified   make.config.in
@@ -300,7 +300,7 @@ $(SOURCEC:%.cxx=%.o): $(LIB)
 $(TARGET): | $(DIRS)
 $(TARGET): makefile $(BOUT_CONFIG_FILE) $(OBJ) $(SUB_LIBS)
 	@echo "  Linking" $(TARGET)
-	@$(LD) $(LDFLAGS) -o $(TARGET) $(OBJ) $(SUB_LIBS) $(BOUT_LIBS)
+	@$(LD) -o $(TARGET) $(OBJ) $(SUB_LIBS) $(BOUT_LIBS) $(LDFLAGS)
 
 checklib:
 ifneq ("$(CHANGED)foo", "foo")

This is to make sure --coverage (which contains -lgcov) appears after -lbout++.

A second issue is that in the current form, --enable-shared --enable-static doesn't work:

Creating libbout++.so
Recreating libbout++.a
ar: ../lib/libbout++.a: No such file or directory
make[1]: *** [makefile:61: ../lib/libbout++.a] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [make.config:133: src] Error 2

* Add $(LDFLAGS) to the so creation
* LIB_A should be created as static archive, never the so
* If creating the lib fails, we don't want to keep the broken lib
* LDFLAGS should be at the end when linking
@dschwoerer
Copy link
Contributor Author

Thanks for finding these issues 👍
Builds are passing now

@ZedThree ZedThree changed the title shared library Fix --enabled-shared for configure Apr 3, 2020
@ZedThree ZedThree merged commit c5dd386 into next Apr 3, 2020
@ZedThree ZedThree deleted the ds-travis branch April 3, 2020 13:34
@ZedThree
Copy link
Member

ZedThree commented Apr 3, 2020

Thanks for sorting this out 🎉

Just to note that there were a bunch of unrelated changes to configure, likely from having different developers having different versions of autoconf on their machines. Probably why they say don't have configure under version control!

@dschwoerer
Copy link
Contributor Author

Yes, I sometimes try to minimise the changes, but not always. If this is something that bothers you, I can try to pay more attention / use whatever autoconf we determine as official one ...

@ZedThree
Copy link
Member

ZedThree commented Apr 6, 2020

It's tricky because there's not been an official autoconf release in about 8 years, but there has been a lot of changes to master, plus distro-specific patches, which means unless we all use the exact same version of a particular distro (or build it from source ourselves!), there's not much chance of us getting the same version.

Having said that, someone has secured funding to try and release 2.70 hopefully soonish.

To be honest, unintended changes are not a major problem, it's just tricky to work out if a change has been "forward" or "backward"!

@loeiten
Copy link
Member

loeiten commented Apr 10, 2020

Hi

It seems like I'm unable to configure after this change, although it looks like it's working fine in 958f685 (the next commit before this merge)

Now

~/BOUT-dev # ./configure --prefix=/root/local --enable-checks=no --enable-optimize=3 --with-fftw=/root/local --with-netcdf=/root/local --with-hdf5=/root/local/bin/h5cc --with-sundials=/root/local --with-petsc=/root/local --with-slepc=/root/local

yields

checking for mpic++... mpic++
checking whether the C++ compiler works... yes
checking for C++ compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C++ compiler... yes
checking whether mpic++ accepts -g... yes
checking for function MPI_Init... yes
checking for mpi.h... yes
checking for a thread-safe mkdir -p... build-aux/install-sh -c -d
checking whether ln -s works... yes
checking whether make sets $(MAKE)... yes
checking for a BSD-compatible install... /usr/bin/install -c
checking for gmake... make
checking for ranlib... ranlib
checking whether mpic++ supports C++14 features with -std=c++14... yes
checking for sqrt in -lm... yes
checking how to run the C++ preprocessor... mpic++ -std=c++14 -E
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for ANSI C header files... yes
checking for sys/types.h... yes
checking for sys/stat.h... yes
checking for stdlib.h... yes
checking for string.h... yes
checking for memory.h... yes
checking for strings.h... yes
checking for inttypes.h... yes
checking for stdint.h... yes
checking for unistd.h... yes
checking malloc.h usability... yes
checking malloc.h presence... yes
checking for malloc.h... yes
checking for stdlib.h... (cached) yes
checking for string.h... (cached) yes
checking for strings.h... (cached) yes
checking for stdlib.h... (cached) yes
checking for GNU libc compatible malloc... yes
checking for stdlib.h... (cached) yes
checking for GNU libc compatible realloc... yes
checking for vprintf... yes
checking for _doprnt... no
checking does C++ compiler support __PRETTY_FUNCTION__... yes
checking for a sed that does not truncate output... /bin/sed
checking whether to build with code coverage support... no
checking whether C++ compiler accepts -Werror=unknown-warning-option... no
checking whether C++ compiler accepts -we10006,10148... no
checking whether C++ compiler accepts -Wall... yes
checking whether C++ compiler accepts -Wextra... yes
checking whether C++ compiler accepts -Wnull-dereference... yes
checking whether C++ compiler accepts -Wno-cast-function-type... yes
configure: Enabling level 3 optimisations
checking whether C++ compiler accepts -O3... yes
checking whether C++ compiler accepts -march=native... yes
checking whether C++ compiler accepts -funroll-loops... yes
configure: Run-time checking disabled
configure: Segmentation fault handling enabled
configure: Output coloring enabled
configure: Field name tracking disabled
configure: Signaling floating point exceptions disabled
checking for addr2line... yes
checking for popen... yes
checking for backtrace... no
configure: WARNING: Native backtrace disabled
./configure: line 6191: LIB_TO_BUILD+= $(BOUT_LIB_PATH)/libbout++.a: not found
configure: error: Need to enable at least on of static or shared!

Trying something similar to the travis script (as far as I'm able to read from the travis build)

/configure --enable-shared --enable-checks=no --enable-optimize=3 --disable-signal --disable
-track --disable-backtrace

and even

~/BOUT-dev # ./configure

yields the same.

I'm running on a ubuntu:latest image. Any ideas on how I can configure @dschwoerer or @ZedThree?

@dschwoerer
Copy link
Contributor Author

dschwoerer commented Apr 10, 2020 via email

@loeiten
Copy link
Member

loeiten commented Apr 11, 2020

Oh, sorry @dschwoerer. I mixed my github action running on ubuntu:latest from my dev environment running on loeiten/bout_dev:latest.
The error persist on my dev environment after this merge, but not on the commits before.

My dev environment is basically alpine with packages like fftw built from scratch.

Reading from the config.log, it looks like a test where confdefs.h fails on return backtrace (); due to WARNING: Native backtrace disabled.

Do you think this error is compiler related?

/BOUT-dev # gcc --version
gcc (Alpine 9.2.0) 9.2.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

@dschwoerer
Copy link
Contributor Author

dschwoerer commented Apr 11, 2020 via email

@dschwoerer
Copy link
Contributor Author

There is gcc 9.3, which is a bug fix release for 9.2, so wouldn't hurt to upgrade.
It's been released nearly a month ago, so hopefully alpine fixes that soon.

However, I strongly doubt we are doing any thing that has a fair chance of triggering a bug during configure -.-

I guess I never tested the case where native backtrace is not available ... so will dig into it ...

@dschwoerer
Copy link
Contributor Author

dschwoerer commented Apr 11, 2020

I tried again, but couldn't get gcc on alpine to work:

apk add gcc
echo "int main(){return 0;} 
" > test.c
gcc test.c

However, gcc fails with

/usr/lib/gcc/x86_64-alpine-linux-musl/9.2.0/../../../../x86_64-alpine-linux-musl/bin/ld: cannot find Scrt1.o: No such file or directory
/usr/lib/gcc/x86_64-alpine-linux-musl/9.2.0/../../../../x86_64-alpine-linux-musl/bin/ld: cannot find crti.o: No such file or directory
/usr/lib/gcc/x86_64-alpine-linux-musl/9.2.0/../../../../x86_64-alpine-linux-musl/bin/ld: cannot find -lssp_nonshared
collect2: error: ld returned 1 exit status

And without a compiler I cannot even build & install any missing dependencies -.-

@loeiten
Copy link
Member

loeiten commented Apr 11, 2020

Thanks for helping out @dschwoerer.
The build file can be found in this image.

Running

apk add gcc
echo "int main(){return 0;} 
" > test.c
gcc test.c

works without throwing the errors you mention above. Could it be that you are missing the build-base package (https://wiki.alpinelinux.org/wiki/GCC)?

@dschwoerer
Copy link
Contributor Author

dschwoerer commented Apr 11, 2020 via email

@dschwoerer
Copy link
Contributor Author

dschwoerer commented Apr 11, 2020 via email

@loeiten
Copy link
Member

loeiten commented Apr 11, 2020

Awesome 🥳, thanks 🎉.
If you are interested in using the image, you can get it by calling

docker pull loeiten/bout_dev:latest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants