argon2: use as submodule #175

Merged
merged 1 commit into from Mar 27, 2017

Conversation

Projects
None yet
2 participants
@sim590
Contributor

sim590 commented Feb 28, 2017

I have made sure we use ref.c from argon2 as it is the case right now in master.

@sim590 sim590 requested review from aberaud and yomgui1 Feb 28, 2017

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Mar 1, 2017

Contributor

N.B: this patch also bumps the source to the latest master.

Contributor

sim590 commented Mar 1, 2017

N.B: this patch also bumps the source to the latest master.

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Mar 1, 2017

Contributor
  • [ ] add CMake and Autotools checks that argon2 repository is initialized.

Notice the procedure in README.mdwiki (git submodule update --init) once this is merged.

Contributor

sim590 commented Mar 1, 2017

  • [ ] add CMake and Autotools checks that argon2 repository is initialized.

Notice the procedure in README.mdwiki (git submodule update --init) once this is merged.

@aberaud

This comment has been minimized.

Show comment
Hide comment
@aberaud

aberaud Mar 13, 2017

Contributor

The goal of including argon2 is to make life easier for users while argon2 is progressively added to distributions. Since argon2 is now in Debian and soon Ubuntu, I added support to use the system library instead (used by default, with a fallback on the included version).

  • this PR makes the build depend on internal file structure of argon2 so it should specify a commit (if argon2 file structure changes, OpenDHT build would break).
  • make sure it's compatible as a fallback if the system library is not found (with include <argon2.h> - small rebase needed, sorry).
  • can we init the submodule automatically only if pkgconfig doesn't find the system argon2 ? that would be the most convenient for users. The more we move on, more users will have an argon2 system library ; we shouldn't tell most users to init submodules if they don't have to.

To be honest IMHO the current way is simpler - just keeping a copy that we update - for the next few months, until we can simply drop the included version. Latest commits ensure people with a system version can use it.

Contributor

aberaud commented Mar 13, 2017

The goal of including argon2 is to make life easier for users while argon2 is progressively added to distributions. Since argon2 is now in Debian and soon Ubuntu, I added support to use the system library instead (used by default, with a fallback on the included version).

  • this PR makes the build depend on internal file structure of argon2 so it should specify a commit (if argon2 file structure changes, OpenDHT build would break).
  • make sure it's compatible as a fallback if the system library is not found (with include <argon2.h> - small rebase needed, sorry).
  • can we init the submodule automatically only if pkgconfig doesn't find the system argon2 ? that would be the most convenient for users. The more we move on, more users will have an argon2 system library ; we shouldn't tell most users to init submodules if they don't have to.

To be honest IMHO the current way is simpler - just keeping a copy that we update - for the next few months, until we can simply drop the included version. Latest commits ensure people with a system version can use it.

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Mar 14, 2017

Contributor

Hi @aberaud,

this PR makes the build depend on internal file structure of argon2 so it should specify a commit (if argon2 file structure changes, OpenDHT build would break).

The submodule itself keeps track of the commit to be checked out. If you clone the OpenDHT repository and enter git submodule update --init, you'll end up with the same HEAD. When commited, a submodule is binded to a commit, so it won't move and break without us moving it.

can we init the submodule automatically only if pkgconfig doesn't find the system argon2 ?

I guess we could do git submodule init automatically, but not git submodule update --init as that could be too intrusive in the user's workflow I imagine. Tell me if you think otherwise.

To be honest IMHO the current way is simpler - just keeping a copy that we update - for the next few months, until we can simply drop the included version.

Doing so prevents us to know what commit was used and it's simply harder to maintain if we have to. Plus, my patch is ready for the most part, so... It's just better to merge it.

Latest commits ensure people with a system version can use it.

That is not the issue. The issue is, as I said above, that the devs (us) cannot know what revision was used. It is troublesome when comes the time to choose another version because if, for instance, upstream says it's better. Note also that the submodule uses the latest master (as of when I uploaded the patch). To untar an archive into the repository is not guaranteeing the latest version for people to use it more than using a submodule. Actually, submodule is better here as I said.

I'll address the points listed according to my comments in the following hours/day.

Contributor

sim590 commented Mar 14, 2017

Hi @aberaud,

this PR makes the build depend on internal file structure of argon2 so it should specify a commit (if argon2 file structure changes, OpenDHT build would break).

The submodule itself keeps track of the commit to be checked out. If you clone the OpenDHT repository and enter git submodule update --init, you'll end up with the same HEAD. When commited, a submodule is binded to a commit, so it won't move and break without us moving it.

can we init the submodule automatically only if pkgconfig doesn't find the system argon2 ?

I guess we could do git submodule init automatically, but not git submodule update --init as that could be too intrusive in the user's workflow I imagine. Tell me if you think otherwise.

To be honest IMHO the current way is simpler - just keeping a copy that we update - for the next few months, until we can simply drop the included version.

Doing so prevents us to know what commit was used and it's simply harder to maintain if we have to. Plus, my patch is ready for the most part, so... It's just better to merge it.

Latest commits ensure people with a system version can use it.

That is not the issue. The issue is, as I said above, that the devs (us) cannot know what revision was used. It is troublesome when comes the time to choose another version because if, for instance, upstream says it's better. Note also that the submodule uses the latest master (as of when I uploaded the patch). To untar an archive into the repository is not guaranteeing the latest version for people to use it more than using a submodule. Actually, submodule is better here as I said.

I'll address the points listed according to my comments in the following hours/day.

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Mar 15, 2017

Contributor

I guess we could do git submodule init automatically, but not git submodule update --init as that could be too intrusive in the user's workflow I imagine. Tell me if you think otherwise.

Actually, it doesn't make sense what I said there. update --init has to be run ultimately if the submodule is needed. Also, I guess it's fine if we automatically update --init.

Contributor

sim590 commented Mar 15, 2017

I guess we could do git submodule init automatically, but not git submodule update --init as that could be too intrusive in the user's workflow I imagine. Tell me if you think otherwise.

Actually, it doesn't make sense what I said there. update --init has to be run ultimately if the submodule is needed. Also, I guess it's fine if we automatically update --init.

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Mar 15, 2017

Contributor

can we init the submodule automatically only if pkgconfig doesn't find the system argon2 ?

@aberaud: What if someone uses make dist to generate a tar ball and that git submodule update --init is run from within the configure file? If it was to be automated, it would have to be written inside autogen.sh, then the submodule wil always be updated and init, but I don't mind this. I'm okay witth that. Are you?

Contributor

sim590 commented Mar 15, 2017

can we init the submodule automatically only if pkgconfig doesn't find the system argon2 ?

@aberaud: What if someone uses make dist to generate a tar ball and that git submodule update --init is run from within the configure file? If it was to be automated, it would have to be written inside autogen.sh, then the submodule wil always be updated and init, but I don't mind this. I'm okay witth that. Are you?

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Mar 15, 2017

Contributor

@aberaud: I have addressed your list of concerns. Tell me what you think.

Contributor

sim590 commented Mar 15, 2017

@aberaud: I have addressed your list of concerns. Tell me what you think.

@aberaud

aberaud requested changes Mar 15, 2017 edited

OK, I'm ready to merge this if it works right.

The most important issue is that argon2 submodule headers are included (with cmake and autotools) and library built and linked (with autotools) even if the system argon2 is installed and supposedly used.

Also make sure that we can properly fallback to the included argon2 if the system argon2 is not found. The only case where it should fail is with autotools, if --with-argon2 is passed and system argon is not found.

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Mar 16, 2017

Contributor

The most important issue is that argon2 submodule headers are included (with cmake and autotools) and library built and linked (with autotools) even if the system argon2 is installed and supposedly used.

What do you mean by "included"? If you only mean that it should build with the appropriate headers specified by the platform, then yes it will as per this line, this line and this line.

Also make sure that we can properly fallback to the included argon2 if the system argon2 is not found. The only case where it should fail is with autotools, if --with-argon2 is passed and system argon is not found.

Then, you don't want to make the user have to pass the --without-argon2 flag himself? I think your change in 358d8bd already introduced this behavior. I have tested on my machine and it works well on either build systems. In my change, I have used what you wrote when I reabsed my PR.

Contributor

sim590 commented Mar 16, 2017

The most important issue is that argon2 submodule headers are included (with cmake and autotools) and library built and linked (with autotools) even if the system argon2 is installed and supposedly used.

What do you mean by "included"? If you only mean that it should build with the appropriate headers specified by the platform, then yes it will as per this line, this line and this line.

Also make sure that we can properly fallback to the included argon2 if the system argon2 is not found. The only case where it should fail is with autotools, if --with-argon2 is passed and system argon is not found.

Then, you don't want to make the user have to pass the --without-argon2 flag himself? I think your change in 358d8bd already introduced this behavior. I have tested on my machine and it works well on either build systems. In my change, I have used what you wrote when I reabsed my PR.

@aberaud

More precisions about the issues

CMakeLists.txt
@@ -118,6 +122,7 @@ configure_file (
include_directories (
./
+ argon2/include/

This comment has been minimized.

@aberaud

aberaud Mar 21, 2017

Contributor

Always includes local argon2 headers even when system argon2 is used

@aberaud

aberaud Mar 21, 2017

Contributor

Always includes local argon2 headers even when system argon2 is used

This comment has been minimized.

@sim590

sim590 Mar 21, 2017

Contributor

What's the purpose of this?

EDIT: Oh, you are actually acknowledging something rather than requesting. So you don't want to include if the system argon2 is used. I thought you meant the opposite by the look of your sentence.

@sim590

sim590 Mar 21, 2017

Contributor

What's the purpose of this?

EDIT: Oh, you are actually acknowledging something rather than requesting. So you don't want to include if the system argon2 is used. I thought you meant the opposite by the look of your sentence.

This comment has been minimized.

@aberaud

aberaud Mar 21, 2017

Contributor

Indeed. Now that you mention it, I see it wasn't so clear.

@aberaud

aberaud Mar 21, 2017

Contributor

Indeed. Now that you mention it, I see it wasn't so clear.

CMakeLists.txt
@@ -34,6 +34,10 @@ if (NOT OPENDHT_ARGON2)
message("Argon2 not found, using included version.")
set(OPENDHT_ARGON2 ON)
endif()
+else()
+ # make sure argon2 submodule is up to date and initialized
+ message("Initializing Argon2 submodule")

This comment has been minimized.

@aberaud

aberaud Mar 21, 2017

Contributor

Not run in case of fallback (line 34)

@aberaud

aberaud Mar 21, 2017

Contributor

Not run in case of fallback (line 34)

This comment has been minimized.

@sim590

sim590 Mar 21, 2017

Contributor

Fixed.

@sim590

sim590 Mar 21, 2017

Contributor

Fixed.

@@ -1 +1,2 @@
+git submodule update --init

This comment has been minimized.

@aberaud

aberaud Mar 21, 2017

Contributor

run even if system argon2 is used. I suggest to run only of if argon2 directory doesn't exist.

@aberaud

aberaud Mar 21, 2017

Contributor

run even if system argon2 is used. I suggest to run only of if argon2 directory doesn't exist.

This comment has been minimized.

@sim590

sim590 Mar 21, 2017

Contributor

I suggest to run only of if argon2 directory doesn't exist.

There is a case where the directory doesn't exist and system argon2 submodule should be used. It seems like the second sentence doesn't answer the issue in the former. Anyway, checking if directory exists doesn't change anything since git will do that for us.

If Instead, we were to move the init call into the configure.ac spec, then the call would be part of the configure script. Therefore, the git call would occur even when run from a distributed tarball where the git repository wouldn't exist. As a quick example:

$ make dist
$ tar -xzf opendht-${version}.tar.gz
$ cd opendht-${version} && ./configure

The last line would give error. That's the reason why I decided to put the call into autogen.sh since it is a shell command that belongs into the repository scripts and not in the distributed scripts as I said earlier in other comments. This is a valid point since it is part of the features of autotools which we support. One could ask "what happens if the autogen.sh script is run outside of the repository" to which I would answer: "this is not a supported case since it's not handled by autotools and such case would only happen if the user would pull out the files from the repository which is not the way opendht is intended to be built."

@sim590

sim590 Mar 21, 2017

Contributor

I suggest to run only of if argon2 directory doesn't exist.

There is a case where the directory doesn't exist and system argon2 submodule should be used. It seems like the second sentence doesn't answer the issue in the former. Anyway, checking if directory exists doesn't change anything since git will do that for us.

If Instead, we were to move the init call into the configure.ac spec, then the call would be part of the configure script. Therefore, the git call would occur even when run from a distributed tarball where the git repository wouldn't exist. As a quick example:

$ make dist
$ tar -xzf opendht-${version}.tar.gz
$ cd opendht-${version} && ./configure

The last line would give error. That's the reason why I decided to put the call into autogen.sh since it is a shell command that belongs into the repository scripts and not in the distributed scripts as I said earlier in other comments. This is a valid point since it is part of the features of autotools which we support. One could ask "what happens if the autogen.sh script is run outside of the repository" to which I would answer: "this is not a supported case since it's not handled by autotools and such case would only happen if the user would pull out the files from the repository which is not the way opendht is intended to be built."

This comment has been minimized.

@aberaud

aberaud Mar 22, 2017

Contributor

what about putting the .gitmodule file in the dist archive ?

@aberaud

aberaud Mar 22, 2017

Contributor

what about putting the .gitmodule file in the dist archive ?

This comment has been minimized.

@sim590

sim590 Mar 22, 2017

Contributor

The .git directory wouldn't even be included in the dist archive since it is not known by autotools. Therefore, git commands wouldn't work.

@sim590

sim590 Mar 22, 2017

Contributor

The .git directory wouldn't even be included in the dist archive since it is not known by autotools. Therefore, git commands wouldn't work.

src/Makefile.am
+# ARGON2 submodule #
+######################
+
+noinst_LTLIBRARIES = libargon2.la

This comment has been minimized.

@aberaud

aberaud Mar 21, 2017

Contributor
  • built even if system argon2 is used (check with WITH_INCLUDED_ARGON2 macro)
  • dependency issue: bug can be reproduced easily without parallel build: make clean and make (without -j):

../libtool: line 7485: cd: ../argon2/src/.libs: No such file or directory
libtool: error: cannot determine absolute directory name of '../argon2/src/.libs'

@aberaud

aberaud Mar 21, 2017

Contributor
  • built even if system argon2 is used (check with WITH_INCLUDED_ARGON2 macro)
  • dependency issue: bug can be reproduced easily without parallel build: make clean and make (without -j):

../libtool: line 7485: cd: ../argon2/src/.libs: No such file or directory
libtool: error: cannot determine absolute directory name of '../argon2/src/.libs'

This comment has been minimized.

@sim590

sim590 Mar 21, 2017

Contributor
  • built even if system argon2 is used (check with WITH_INCLUDED_ARGON2 macro)

Still on it. I'll update when done.

  • dependency issue: bug can be reproduced easily without parallel build: make clean and make (without -j)

../libtool: line 7485: cd: ../argon2/src/.libs: No such file or directory
libtool: error: cannot determine absolute directory name of '../argon2/src/.libs'

I don't experience this on 4646756. Logically, this should not happen because of the first point you mentionned. The path ../argon2/src/.libs should always be found. Are you sure you're on the good revision?

@sim590

sim590 Mar 21, 2017

Contributor
  • built even if system argon2 is used (check with WITH_INCLUDED_ARGON2 macro)

Still on it. I'll update when done.

  • dependency issue: bug can be reproduced easily without parallel build: make clean and make (without -j)

../libtool: line 7485: cd: ../argon2/src/.libs: No such file or directory
libtool: error: cannot determine absolute directory name of '../argon2/src/.libs'

I don't experience this on 4646756. Logically, this should not happen because of the first point you mentionned. The path ../argon2/src/.libs should always be found. Are you sure you're on the good revision?

This comment has been minimized.

@sim590

sim590 Mar 21, 2017

Contributor

I guess there may have a src/libargon2.la file lying in your directory. This is due to the switch between system and included argon2. To help preventing this, I'll add a clean-local rule.

@sim590

sim590 Mar 21, 2017

Contributor

I guess there may have a src/libargon2.la file lying in your directory. This is due to the switch between system and included argon2. To help preventing this, I'll add a clean-local rule.

This comment has been minimized.

@sim590

sim590 Mar 21, 2017

Contributor

I don't experience this on 4646756.

I have just reproduced it. I'm on it.

@sim590

sim590 Mar 21, 2017

Contributor

I don't experience this on 4646756.

I have just reproduced it. I'm on it.

This comment has been minimized.

@sim590

sim590 Mar 22, 2017

Contributor

Fixed.

@sim590

sim590 Mar 22, 2017

Contributor

Fixed.

tools/Makefile.am
@@ -4,10 +4,10 @@ noinst_HEADERS = tools_common.h
AM_CPPFLAGS = -I../include
dhtnode_SOURCES = dhtnode.cpp
-dhtnode_LDFLAGS = -lopendht -lreadline -L../src/.libs @GnuTLS_LIBS@
+dhtnode_LDFLAGS = -lopendht -lreadline -L@top_builddir@/src/.libs -L@top_builddir@/argon2/src/.libs @GnuTLS_LIBS@

This comment has been minimized.

@aberaud

aberaud Mar 21, 2017

Contributor

local argon2 path included even when system library is used. might use @argon2_LIBS@ ?

@aberaud

aberaud Mar 21, 2017

Contributor

local argon2 path included even when system library is used. might use @argon2_LIBS@ ?

This comment has been minimized.

@sim590

sim590 Mar 21, 2017

Contributor

Yes. I forgot about the tools in my last patch. Thanks.

@sim590

sim590 Mar 21, 2017

Contributor

Yes. I forgot about the tools in my last patch. Thanks.

This comment has been minimized.

@sim590

sim590 Mar 21, 2017

Contributor

FIxed.

@sim590

sim590 Mar 21, 2017

Contributor

FIxed.

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Mar 22, 2017

Contributor

Now I have a final problem:

/bin/bash ../libtool  --tag=CXX   --mode=link g++  -O3 -pedantic-errors -DMSGPACK_DISABLE_LEGACY_NIL -DMSGPACK_DISABLE_LEGACY_CONVERT -lpthread -L/home/simon/prog/opendht/argon2/src/.libs -lpthread -o libopendht.la -rpath /usr/local/lib libopendht_la-dht.lo libopendht_la-node_cache.lo libopendht_la-callbacks.lo libopendht_la-routing_table.lo libopendht_la-network_engine.lo libopendht_la-utils.lo libopendht_la-infohash.lo libopendht_la-node.lo libopendht_la-value.lo libopendht_la-crypto.lo libopendht_la-securedht.lo libopendht_la-dhtrunner.lo libopendht_la-default_types.lo indexation/libopendht_la-pht.lo libopendht_la-log.lo  libargon2.la -lgnutls -lnettle
libtool:   error: cannot find the library 'libargon2.la' or unhandled argument 'libargon2.la'

So, from what I can understand, this is due to those three lines in Makefile.am:

lib_LTLIBRARIES = libopendht.la
libopendht_la_LIBADD = @Argon2_LIBS@ @GnuTLS_LIBS@ @Nettle_LIBS@
noinst_LTLIBRARIES = libargon2.la

It seems like *_LIBADD doesn't specify dependencies or an order in which to build things. So, libtool tries to link with libargon2.la which has not been built yet. It thinks it can build it after building libopendht.so. In master, we go around the issue litterally by using subdirs makefiles. Since argon2 subdir is specified before src, the problem doesn't occur. However, there should be a way to specify the dependency. I'm still looking for it. The only thing that resolves this issue for now is using lib_LTLIBRARIES = libargon2.la libopendht.la, which is not suited since we don't want to install libargon2.

@aberaud: Do you have a suggestion?

Contributor

sim590 commented Mar 22, 2017

Now I have a final problem:

/bin/bash ../libtool  --tag=CXX   --mode=link g++  -O3 -pedantic-errors -DMSGPACK_DISABLE_LEGACY_NIL -DMSGPACK_DISABLE_LEGACY_CONVERT -lpthread -L/home/simon/prog/opendht/argon2/src/.libs -lpthread -o libopendht.la -rpath /usr/local/lib libopendht_la-dht.lo libopendht_la-node_cache.lo libopendht_la-callbacks.lo libopendht_la-routing_table.lo libopendht_la-network_engine.lo libopendht_la-utils.lo libopendht_la-infohash.lo libopendht_la-node.lo libopendht_la-value.lo libopendht_la-crypto.lo libopendht_la-securedht.lo libopendht_la-dhtrunner.lo libopendht_la-default_types.lo indexation/libopendht_la-pht.lo libopendht_la-log.lo  libargon2.la -lgnutls -lnettle
libtool:   error: cannot find the library 'libargon2.la' or unhandled argument 'libargon2.la'

So, from what I can understand, this is due to those three lines in Makefile.am:

lib_LTLIBRARIES = libopendht.la
libopendht_la_LIBADD = @Argon2_LIBS@ @GnuTLS_LIBS@ @Nettle_LIBS@
noinst_LTLIBRARIES = libargon2.la

It seems like *_LIBADD doesn't specify dependencies or an order in which to build things. So, libtool tries to link with libargon2.la which has not been built yet. It thinks it can build it after building libopendht.so. In master, we go around the issue litterally by using subdirs makefiles. Since argon2 subdir is specified before src, the problem doesn't occur. However, there should be a way to specify the dependency. I'm still looking for it. The only thing that resolves this issue for now is using lib_LTLIBRARIES = libargon2.la libopendht.la, which is not suited since we don't want to install libargon2.

@aberaud: Do you have a suggestion?

./
include/
include/opendht/
${CMAKE_CURRENT_BINARY_DIR}/include/
)
+if(OPENDHT_ARGON2)

This comment has been minimized.

@aberaud

aberaud Mar 22, 2017

Contributor

group with other if(OPENDHT_ARGON2) directives here

@aberaud

aberaud Mar 22, 2017

Contributor

group with other if(OPENDHT_ARGON2) directives here

This comment has been minimized.

@sim590

sim590 Mar 22, 2017

Contributor

Fixed.

@sim590

sim590 Mar 22, 2017

Contributor

Fixed.

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Mar 22, 2017

Contributor

Thanks to @yomgui1 and a quite long search before, I have found:

noinst_LTLIBRARIES = libargon2.la
libopendht_la_DEPENDENCIES = libargon2.la

which solves the dependency issue. This is actually needed and should have been done from the begining (in master). However, as I said, we go around the problem by using the subdirs order in the main Makefile.am file.

Contributor

sim590 commented Mar 22, 2017

Thanks to @yomgui1 and a quite long search before, I have found:

noinst_LTLIBRARIES = libargon2.la
libopendht_la_DEPENDENCIES = libargon2.la

which solves the dependency issue. This is actually needed and should have been done from the begining (in master). However, as I said, we go around the problem by using the subdirs order in the main Makefile.am file.

@aberaud aberaud merged commit c424eb9 into savoirfairelinux:master Mar 27, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

aberaud added a commit that referenced this pull request Mar 29, 2017

Merge pull request #175 from sim590/argon2-submodule
argon2: use as submodule (reverted from commit c424eb9)

aberaud added a commit that referenced this pull request Mar 30, 2017

Merge pull request #175 from sim590/argon2-submodule
argon2: use as submodule (reverted from commit c424eb9) (reverted from commit 3c45e19)

amirouche added a commit to amirouche/opendht that referenced this pull request Apr 11, 2017

Merge pull request #175 from sim590/argon2-submodule
argon2: use as submodule (reverted from commit c424eb9)

amirouche added a commit to amirouche/opendht that referenced this pull request Apr 11, 2017

Merge pull request #175 from sim590/argon2-submodule
argon2: use as submodule (reverted from commit c424eb9) (reverted from commit 3c45e19)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment