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

WIP: ./configure #1538

Merged
merged 10 commits into from
Jun 8, 2018
Merged

Conversation

rustyrussell
Copy link
Contributor

[ On top of gossip changes, so ignore all patches up to and including 'gossipd: minor cleanups.' ]

This adds a ./configure step for those of us who hate remembering to 'make DEVELOPER=1'. It's not autoconf, but if you can write shell it should be pretty simple to enhance.

Main user-visible change is that you need to use 'VALGRIND=0' instead of 'NO_VALGRIND=1' to override valgrind testing.

@jsarenik might want to comment, since he's doing exotic cross-builds!

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 4, 2018

Will check it out today. Thanks!

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 4, 2018 via email

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 4, 2018

Already compiling. Not a lot of success yet. I have a note which is not really related to this particular HEAD, but it is still present in it: There is a race condition when make -j# is issued (where # is a natural number higher than two, e.g. 4).

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 4, 2018

Two things. Here is the fix for the race condition. Start with cleanly cloned lightning repository. The fix lies in initiating the git submodules early and in non-simultaneous way by calling

git submodule update --init

There could be also --recursive but it is not needed in our case (yet). By the way this can be simplified into one clone command as (excuse the usage of jsarenik fork, but for the sake of a working Alpine Linux example):

lomidrevo:/tmp$ rm -rf lightning; git clone https://github.com/jsarenik/lightning -b rusty-guilt-configure --recurse-submodules
...
lomidrevo:/tmp$ cd lightning
lomidrevo:/tmp/lightning$ ./configure --enable-developer --disable-valgrind
...

Then after make -j4 I have a nicely compiled and perfectly working lightning. My conclusion is that no git magic should happen from inside the Makefiles.

Try to run the same as above, but without the --recurse-submodules. I get unfinished makes so that I can run while ! make; do :; done and still do not get any successfully finished make. See #1542.

The other thing is that only the latest config.guess and config.sub fixes the compilation issue for me on Alpine Linux. I have sent a PR rustyrussell/libbacktrace#2

And have checked it locally with changing your (@rustyrussell) commit to (already found in above-mentioned jsarenik/rusty-guilt-compile branch):

diff --git a/.gitmodules b/.gitmodules
index cada6332..c0410770 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -9,7 +9,7 @@
        url = https://github.com/jedisct1/libsodium.git
 [submodule "external/libbacktrace"]
        path = external/libbacktrace
-       url = https://github.com/ianlancetaylor/libbacktrace.git
+       url = https://github.com/jsarenik/libbacktrace.git
 [submodule "external/libwally-core"]
        path = external/libwally-core
        url = https://github.com/ElementsProject/libwally-core.git
diff --git a/external/libbacktrace b/external/libbacktrace
index 14d377e9..6aecc10c 160000
--- a/external/libbacktrace
+++ b/external/libbacktrace
@@ -1 +1 @@
-Subproject commit 14d377e9be7c89511c472d728e1b88b4e96f1946
+Subproject commit 6aecc10cb6830fcb04d890c7f907c91bb5e65dea

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 4, 2018

Oh, by the way my so-called race condition is not really a race condition because it happens even when make is called without the -j flag. The test (while ! make; do :; done) really did not end after many rounds...

Here is how to fix it in the same directory just after pressing ^C breaking the infinite failure loop:

git submodule deinit --all -f
git submodule update --init

Now run make, even parallel (with -j4) and it compiles on the first try. Yeah.

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 4, 2018

And because I am knowingly mixing two issues here, I will make a new one for the "race condition" with a full log.

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 4, 2018

The race-condition issue is fully documented in #1543

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 5, 2018

Upstream libbacktrace already updated. I've sent a PR to this branch. rustyrussell#2

@rustyrussell
Copy link
Contributor Author

"This time for sure"!

@bitonic-cjp
Copy link
Contributor

bitonic-cjp commented Jun 6, 2018

See #1544: I have a feature request for the configure script: choose between pytest-3 and pytest (whichever is available; use pytest-3 if both exist and user didn't manually override).

What is the preferred way to do this? Try to make it myself and make a pull request for rustyrussell/lightning/tree/guilt/configure ? Wait until someone else (@rustyrussell ?) adds this? Something else?

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 6, 2018

Oops. Seems like there is some problem when I do a clean clone and build. See further comments...

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 6, 2018

@rustyrussell The whole problem lies in checksum for libbacktrace, which was changed to the tip in your fork (rustyrussell/libbacktrace@078f841) in f660271, but the submodule uses the original repository which does not contain that commit.

Here is the ammended and corrected commit:

From acd087192ea0dd84b3694796f61110d1a6669463 Mon Sep 17 00:00:00 2001
From: Rusty Russell <[email protected]>
Date: Wed, 6 Jun 2018 16:11:49 +0930
Subject: [PATCH] libbacktrace: pull latest version, which has config.guess fix
 applied.

Signed-off-by: Rusty Russell <[email protected]>
---
 external/libbacktrace | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/external/libbacktrace b/external/libbacktrace
index 14d377e9..5a99ff7f 160000
--- a/external/libbacktrace
+++ b/external/libbacktrace
@@ -1 +1 @@
-Subproject commit 14d377e9be7c89511c472d728e1b88b4e96f1946
+Subproject commit 5a99ff7fed66b8ea8f09c9805c138524a7035ece
-- 
2.17.1

After this is applied instead of f660271 and the code is rebased to the current master branch, and VALGRIND=0 is change is done also in Python tests, then as for me and my house it is ready to be merged.

Full example:

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 6, 2018

Mystery solved: rustyrussell/libbacktrace@078f841 was your local commit.

I have edited/removed my previous comments accordingly.

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 6, 2018

@bitonic-cjp I would suggest you to wait until this gets merged into master. Then rebase your work on top of it.

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 6, 2018

Another issue is that when configured with --disable-valgrind, the Python tests still try to run with Valgrind.

Fix available in jsarenik@690c366

PR sent to Rusty's branch: rustyrussell#4

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 6, 2018

@rustyrussell @cdecker @ZmnSCPxj

Should the default for Python tests run without environment be with or without Valgrind?
https://github.com/rustyrussell/lightning/blob/guilt/configure/tests/test_lightningd.py#L34

Sorry for spam. I realized this is totally off-topic. Have a look at rustyrussell@b8e26af without which Valgrind is still run for me.

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 6, 2018

Last changes done. Let's call it a day here in the old Prague. Who will test the tests?

https://en.wikipedia.org/wiki/Quis_custodiet_ipsos_custodes%3F

@rustyrussell
Copy link
Contributor Author

Thanks! I rebased and applied your fixes.

Turns out guilt doesn't play well with submodules, but who does?

@jsarenik
Copy link
Collaborator

jsarenik commented Jun 6, 2018

Thanks for sharing! I didn't know about that vulnerability.

…hanges.

If we change an upstream URL, all submodules break.  Users would need
to run 'git submodule sync'.  Note that the libbacktrace fix was merged
upstream so this is no longer necessary, but it's good for future changes.

Also, stress-testing reveals that git submodule fails locking
'.git/config' when run in paralell.  It also segfaults and other
problems.

This is my final attempt to fix submodules; I've wasted far too many
days on obscure problems it creates: I've already lost one copy of my
repo to apparently unfixable submodule preoblems.  The next "fix" will
be to simply import the source code so it works properly.

Reported-by: @jsarenik
Fixes: ElementsProject#1543
Signed-off-by: Rusty Russell <[email protected]>
You can use environment variables or the commandline to set defaults.

It looks very autoconf, but you don't need to learn m4.

Doesn't cover all the obscure flags, but it's easy to extend.

Signed-off-by: Rusty Russell <[email protected]>
We leave VALGRIND env var as an override for testing.

Signed-off-by: Rusty Russell <[email protected]>
Make should only run configure if config.var already exists:

    $ make
    ./configure --reconfigure
    ./configure: 65: .: config.vars: not found
    ./configure --reconfigure
    ./configure: 65: .: config.vars: not found
    Makefile:179: recipe for target 'ccan/config.h' failed
    make: *** [ccan/config.h] Error 2
This way the object file correctly depends on external headers.  Currently
a parallel build on a clean tree can give:

```
In file included from ./common/sphinx.h:6:0,
                 from devtools/onion.c:5:
./bitcoin/pubkey.h:8:10: fatal error: secp256k1.h: No such file or directory
 #include <secp256k1.h>
          ^~~~~~~~~~~~~
compilation terminated.
<builtin>: recipe for target 'devtools/onion.o' failed
```

Signed-off-by: Rusty Russell <[email protected]>
@jsarenik
Copy link
Collaborator

jsarenik commented Jun 8, 2018

ACK 0a094b9 (no idea what the flags mean and if I can ack at all, but here is my 0.00000002 BTC).

@cdecker cdecker merged commit 109b429 into ElementsProject:master Jun 8, 2018
@cdecker
Copy link
Member

cdecker commented Jun 8, 2018

Damn, sorry about that, I didn't see the WIP in the PR title and went ahead and merged it ☹️

@jsarenik
Copy link
Collaborator

Damn, sorry about that, I didn't see the WIP in the PR title and went ahead and merged it

And here I am :-) again

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.

4 participants