Makefile not portable #10

zbeekman · 2017-02-28T04:41:08Z

The default make that ships with the latest Xcode or Mac OS Yosemite does not honor the president going to the most specialized suffix rule: as a result the SIMD compile time flags don't get added and clang chokes trying to compile some of the avx356 code because -mavx2 doesn't get passed. See #4 for additional discussion.

muellermartin · 2017-02-28T14:41:41Z

I think I could have fixed this by changing the order of those suffix rules. See commit 0b97c4e and related PR #4.

cr-marcstevens · 2017-02-28T15:38:04Z

@zbeekman Can you verify this fixes this issue?

This is a rework of PR cr-marcstevens#4 which was split into smaller PRs which where not related to macOS only. This PR waschanged to reflect the changes which were made in the mean time. Compiling on macOS requires following changes to the Makefile: Some checks were introduced to set compiler flags and file extensions required or specific for macOS. Additionally the order of general pattern rules was changed. The sets of patterns to match certain source files for different CPU features are based on suffixes in their filenames. make on Linux seems to pick the most specific rule whereas make shipped with Xcode does pick the first and therefore disregards the remaining rules. This was fixed by rearranging the rules from most specific to most general. This potentially fixes issue cr-marcstevens#10.

johnhawkinson · 2017-03-02T02:55:22Z

The Makefile still seems a bit dodgy. Let me know if I should open a new Issue, but notice these results if I ask Homebrew to do make install into a clean PREFIX after I have previously run make:

==> make HAVEAVX=0 PREFIX=/usr/local/Cellar/sha1dc/HEAD-6b1c12f install
glibtool --tag=CC --mode=link clang -O2 -g -Wall -Werror -Wextra -pedantic -std=c99 -Ilib -DHAVE_MMX -DHAVE_SSE -DNO_HAVE_AVX -DNO_HAVE_NEON -march=native obj_src/main.lo obj_lib/sha1.lo obj_lib/ubc_check.lo obj_lib/ubc_check_verify.lo obj_lib/sha1_simd_mmx64.lo obj_lib/ubc_check_simd_mmx64.lo obj_lib/sha1_simd_sse128.lo obj_lib/ubc_check_simd_sse128.lo -Lbin -ldetectcoll -o bin/sha1dcsum
glibtool: link: clang -O2 -g -Wall -Werror -Wextra -pedantic -std=c99 -Ilib -DHAVE_MMX -DHAVE_SSE -DNO_HAVE_AVX -DNO_HAVE_NEON -march=native obj_src/.libs/main.o obj_lib/.libs/sha1.o obj_lib/.libs/ubc_check.o obj_lib/.libs/ubc_check_verify.o obj_lib/.libs/sha1_simd_mmx64.o obj_lib/.libs/ubc_check_simd_mmx64.o obj_lib/.libs/sha1_simd_sse128.o obj_lib/.libs/ubc_check_simd_sse128.o -o bin/sha1dcsum  -Lbin /private/tmp/sha1dc-20170301-89152-1ixmp42/bin/.libs/libdetectcoll.a 
ln -s sha1dcsum bin/sha1dcsum_partialcoll
ln: bin/sha1dcsum_partialcoll: File exists
make: [sha1dcsum_partialcoll] Error 1 (ignored)
bin/sha1dcsum test/*
a56374e1cf4c3746499bc7c0acb39498ad2ee185  test/sha1_reducedsha_coll.bin
16e96b70000dd1e7c85b8368ee197754400e58ec *coll* test/shattered-1.pdf
e1761773e6a35916d99f891b77663e6405313587 *coll* test/shattered-2.pdf
bin/sha1dcsum_partialcoll test/*
dd39885a2a5d8f59030b451e00cb45da9f9d3828 *coll* test/sha1_reducedsha_coll.bin
d3a1d09969c3b57113fd17b23e01dd3de74a99bb *coll* test/shattered-1.pdf
92246b0b718f4c704d37bb025717cbc66babf102 *coll* test/shattered-2.pdf
install bin/sha1dcsum /usr/local/Cellar/sha1dc/HEAD-6b1c12f/bin
install bin/sha1dcsum_partialcoll /usr/local/Cellar/sha1dc/HEAD-6b1c12f/bin
install bin/libdetectcoll.la /usr/local/Cellar/sha1dc/HEAD-6b1c12f/lib

pb3:Formula jhawk$ ls -ld /usr/local/Cellar/sha1dc/HEAD-6b1c12f/bin
-rwxr-xr-x  1 jhawk  admin  1344340 Mar  1 21:44 /usr/local/Cellar/sha1dc/HEAD-6b1c12f/bin
pb3:Formula jhawk$ ls -ld /usr/local/Cellar/sha1dc/HEAD-6b1c12f/lib
drwxr-xr-x  3 jhawk  admin  102 Mar  1 21:44 /usr/local/Cellar/sha1dc/HEAD-6b1c12f/lib
pb3:Formula jhawk$ ls -l /usr/local/Cellar/sha1dc/HEAD-6b1c12f/lib

Note that:

Despite make having been run, make install rebuilds sha1dcsum and libdetectcoll.a
Nothing mkdir'd $prefix/bin, so instead of installing the binary inside it, the install rule installed the binary as a file named bin.
Installation copied in a .la libtool file. I think, as I suggested in Add support for macOS #4 (comment), you need to use libtool to install the library, ala: $(LIBTOOL) --tag=CC --mode=install install -c bin/libdetectcoll.la $(PREFIX)/lib

I don't think any of these issues are MacOS -specific.

johnhawkinson · 2017-03-04T02:50:14Z

Also, sha1.h is not installed. There is also something wrong with the libtool installation of the library that I don't understand, cf. http://lists.gnu.org/archive/html/libtool/2017-03/msg00000.html

cr-marcstevens · 2017-03-04T04:38:31Z

Can you look at the simplified_c90 branch, I think it fixes a lot and will go to master soon.
It doesn't yet install the headers, we should still add that.

johnhawkinson · 2017-03-04T05:20:25Z

Yes, I've been following it. BTW, it would be great to get a tag, on either branch.
i.e. poke re #11 :)

At present, the simplified_c90 branch:

No longer needs HAVEAVX=0 under MacOS. Also builds much faster (?)
Correctly resolves the first bullet above (rebuilding at install)
Still tries to install sha1dcsum as a file named bin
With that corrected, installs sha1dcsum but not sha1dcsum_partialcoll
Seems to resolve the 3rd bullet (although I'm not sure it's right).
Builds sha1dcsum linked against a shared libdetectcoll under MacOS, which is not what I would have done, but might be a totally valid choice.

==> mkdir -p /usr/local/Cellar/sha1dc/git-58cac9/bin /usr/local/Cellar/sha1dc/git-58cac9/lib
==> make PREFIX=/usr/local/Cellar/sha1dc/git-58cac9 install
glibtool --tag=CC --mode=install install bin/libdetectcoll.la /usr/local/Cellar/sha1dc/git-58cac9/lib
glibtool: install: install bin/.libs/libdetectcoll.0.dylib /usr/local/Cellar/sha1dc/git-58cac9/lib/libdetectcoll.0.dylib
glibtool: install: (cd /usr/local/Cellar/sha1dc/git-58cac9/lib && { ln -s -f libdetectcoll.0.dylib libdetectcoll.dylib || { rm -f libdetectcoll.dylib && ln -s libdetectcoll.0.dylib libdetectcoll.dylib; }; })
glibtool: install: install bin/.libs/libdetectcoll.lai /usr/local/Cellar/sha1dc/git-58cac9/lib/libdetectcoll.la
glibtool: install: install bin/.libs/libdetectcoll.a /usr/local/Cellar/sha1dc/git-58cac9/lib/libdetectcoll.a
glibtool: install: chmod 644 /usr/local/Cellar/sha1dc/git-58cac9/lib/libdetectcoll.a
glibtool: install: ranlib /usr/local/Cellar/sha1dc/git-58cac9/lib/libdetectcoll.a
glibtool: warning: remember to run 'glibtool --finish /usr/local/lib'
glibtool --tag=CC --mode=install install bin/sha1dcsum /usr/local/Cellar/sha1dc/git-58cac9/bin
glibtool: warning: '/private/tmp/sha1dc-20170304-25067-1onj6r4/bin/libdetectcoll.la' has not been installed in '/usr/local/lib'
glibtool: install: install bin/.libs/sha1dcsum /usr/local/Cellar/sha1dc/git-58cac9/bin/sha1dcsum

cr-marcstevens · 2017-03-04T06:42:09Z

Done another commit to fix install dirs and added header installation.

AVX and others have disappeared, because it was still WIP and still needed a runtime selection.
I've moved this into the simd branch to work on this feature.
Indeed it builds much faster now, also because we only compile strictly necessary recompression functions.

johnhawkinson · 2017-03-04T13:26:04Z

Progress, but install -D is not portable and doesn't work with BSD installs (like MacOS):

glibtool: install: install -D bin/.libs/libsha1detectcoll.0.dylib /usr/local/Cellar/sha1dc/dev-v1.0.1/lib/libsha1detectcoll.0.dylib
install: illegal option -- D
usage: install [-bCcpSsv] [-B suffix] [-f flags] [-g group] [-m mode]
               [-o owner] file1 file2
       install [-bCcpSsv] [-B suffix] [-f flags] [-g group] [-m mode]
               [-o owner] file1 ... fileN directory
       install -d [-v] [-g group] [-m mode] [-o owner] directory ...

Trying to work around it with make INSTALL=ginstall to get GNU install fails because $INSTALL has two meanings, one for "install" and one for libtoolized install:

INSTALL ?= install
...
LT_INSTALL:=$(LIBTOOL) --tag=CC --mode=install $(INSTALL)
...
INSTALL=$(LT_INSTALL)
...
install: all
        $(INSTALL) -D bin/libsha1detectcoll.$(LIB_EXT) $(LIBDIR)/libsha1detectcoll.$(LIB_EXT)

So the result is installation of libtool wrapper scripts:

=> make LIBTOOL=glibtool INSTALL=ginstall PREFIX=/usr/local/Cellar/sha1dc/dev-v1.0.1 install
ginstall -D bin/libsha1detectcoll.la /usr/local/Cellar/sha1dc/dev-v1.0.1/lib/libsha1detectcoll.la
ginstall -D lib/sha1.h /usr/local/Cellar/sha1dc/dev-v1.0.1/include/sha1dc/sha1.h
ginstall -D bin/sha1dcsum /usr/local/Cellar/sha1dc/dev-v1.0.1/bin/sha1dcsum
ginstall -D bin/sha1dcsum_partialcoll /usr/local/Cellar/sha1dc/dev-v1.0.1/bin/sha1dcsum_partialcoll
...
pb3:~ jhawk$ head /usr/local/Cellar/sha1dc/dev-v1.0.1/bin/sha1dcsum
#! /bin/sh

# bin/sha1dcsum - temporary wrapper script for .libs/sha1dcsum
# Generated by libtool (GNU libtool) 2.4.6
#
# The bin/sha1dcsum program cannot be directly executed until all the libtool
# libraries that it depends on are installed.
#
# This wrapper script should never be moved out of the build directory.
# If it is, it will not operate correctly.

Working around that with a Makefile edit, things work! Although there are warnings on install that may or may not matter (I don't understand libtool well enough):

==> make LIBTOOL=glibtool PREFIX=/usr/local/Cellar/sha1dc/dev-v1.0.1 install
glibtool --tag=CC --mode=install ginstall -D bin/libsha1detectcoll.la /usr/local/Cellar/sha1dc/dev-v1.0.1/lib/libsha1detectcoll.la
glibtool: install: ginstall -D bin/.libs/libsha1detectcoll.0.dylib /usr/local/Cellar/sha1dc/dev-v1.0.1/lib/libsha1detectcoll.0.dylib
glibtool: install: (cd /usr/local/Cellar/sha1dc/dev-v1.0.1/lib && { ln -s -f libsha1detectcoll.0.dylib libsha1detectcoll.dylib || { rm -f libsha1detectcoll.dylib && ln -s libsha1detectcoll.0.dylib libsha1detectcoll.dylib; }; })
glibtool: install: ginstall -D bin/.libs/libsha1detectcoll.lai /usr/local/Cellar/sha1dc/dev-v1.0.1/lib/libsha1detectcoll.la
glibtool: install: ginstall -D bin/.libs/libsha1detectcoll.a /usr/local/Cellar/sha1dc/dev-v1.0.1/lib/libsha1detectcoll.a
glibtool: install: chmod 644 /usr/local/Cellar/sha1dc/dev-v1.0.1/lib/libsha1detectcoll.a
glibtool: install: ranlib /usr/local/Cellar/sha1dc/dev-v1.0.1/lib/libsha1detectcoll.a
glibtool: warning: remember to run 'glibtool --finish /usr/local/lib'
glibtool --tag=CC --mode=install ginstall -D lib/sha1.h /usr/local/Cellar/sha1dc/dev-v1.0.1/include/sha1dc/sha1.h
glibtool: install: ginstall -D lib/sha1.h /usr/local/Cellar/sha1dc/dev-v1.0.1/include/sha1dc/sha1.h
glibtool --tag=CC --mode=install ginstall -D bin/sha1dcsum /usr/local/Cellar/sha1dc/dev-v1.0.1/bin/sha1dcsum
glibtool: warning: '/private/tmp/sha1dc-20170304-38979-1he25ul/sha1collisiondetection-development-v1.0.1/bin/libsha1detectcoll.la' has not been installed in '/usr/local/lib'
glibtool: install: ginstall -D bin/.libs/sha1dcsum /usr/local/Cellar/sha1dc/dev-v1.0.1/bin/sha1dcsum
glibtool --tag=CC --mode=install ginstall -D bin/sha1dcsum_partialcoll /usr/local/Cellar/sha1dc/dev-v1.0.1/bin/sha1dcsum_partialcoll
glibtool: warning: '/private/tmp/sha1dc-20170304-38979-1he25ul/sha1collisiondetection-development-v1.0.1/bin/libsha1detectcoll.la' has not been installed in '/usr/local/lib'
glibtool: install: ginstall -D bin/.libs/sha1dcsum_partialcoll /usr/local/Cellar/sha1dc/dev-v1.0.1/bin/sha1dcsum_partialcoll

but the result certainly works and tests ok.

cr-marcstevens · 2017-03-04T18:54:46Z

Have you tried libtool-less installation?
LIBTOOL=libtool-disabled make install

What are the required edits you've made that also make sense for linux installation?
Use LT_INSTALL instead of INSTALL in install target?
Or should we use := instead of = at some places?

johnhawkinson · 2017-03-04T19:13:25Z

Have you tried libtool-less installation? LIBTOOL=libtool-disabled make install

I had not. That's a solution that works for a static builds, but it's not really the "right" solution, I don't think. But it does work.

What are the required edits you've made that also make sense for linux installation?
Use LT_INSTALL instead of INSTALL in install target?
Or should we use := instead of = at some places?

Merely that there be some way to override INSTALL (at least until you use a portable syntax for creating the $prefix directories, since install -D is not portable). Using LT_INSTALL would be fine, I think, but the real point is don't redefine INSTALL in two places to mean two different things, because it screws up command-line overrides.

cr-marcstevens · 2017-03-05T06:39:22Z

All linux and macosx builds on travisci succeed, can I close this issue?

muellermartin · 2017-03-05T16:51:31Z

For me branch simplified_c90 works.

zbeekman · 2017-03-06T17:07:15Z

Yes, this is fixed for me. Sorry for the slow reply.

muellermartin mentioned this issue Mar 1, 2017

Add support for macOS (rework) #16

Closed

johnhawkinson mentioned this issue Mar 5, 2017

sha1dc git (new formula) Detect SHAttered collisions Homebrew/homebrew-core#10431

Closed

4 tasks

cr-marcstevens closed this as completed Mar 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makefile not portable #10

Makefile not portable #10

zbeekman commented Feb 28, 2017

muellermartin commented Feb 28, 2017

cr-marcstevens commented Feb 28, 2017

johnhawkinson commented Mar 2, 2017

johnhawkinson commented Mar 4, 2017

cr-marcstevens commented Mar 4, 2017

johnhawkinson commented Mar 4, 2017

cr-marcstevens commented Mar 4, 2017

johnhawkinson commented Mar 4, 2017

cr-marcstevens commented Mar 4, 2017

johnhawkinson commented Mar 4, 2017

cr-marcstevens commented Mar 5, 2017

muellermartin commented Mar 5, 2017 •

edited

Loading

zbeekman commented Mar 6, 2017

Makefile not portable #10

Makefile not portable #10

Comments

zbeekman commented Feb 28, 2017

muellermartin commented Feb 28, 2017

cr-marcstevens commented Feb 28, 2017

johnhawkinson commented Mar 2, 2017

johnhawkinson commented Mar 4, 2017

cr-marcstevens commented Mar 4, 2017

johnhawkinson commented Mar 4, 2017

cr-marcstevens commented Mar 4, 2017

johnhawkinson commented Mar 4, 2017

cr-marcstevens commented Mar 4, 2017

johnhawkinson commented Mar 4, 2017

cr-marcstevens commented Mar 5, 2017

muellermartin commented Mar 5, 2017 • edited Loading

zbeekman commented Mar 6, 2017

muellermartin commented Mar 5, 2017 •

edited

Loading