sidestep using the config.guess that comes with a package in ConfigureMake generic easyblock #1506

ocaisa · 2018-09-12T15:38:26Z

No description provided.

houndci-bot · 2018-09-12T15:38:43Z

easybuild/easyblocks/generic/configuremake.py

+        build_type, _ = run_cmd('config_guess')
+        build_type_option = '--build=' + build_type.strip()
+
+        cmd = "%(preconfigopts)s %(cmd_prefix)s./configure %(prefix_opt)s%(installdir)s %(build_type_option)s %(configopts)s" % {


line too long (129 > 120 characters)

micaeljtoliveira · 2018-09-13T06:53:11Z

@ocaisa After fixing the typo in the script invocation (config.guess instead of config_guess), I tested this on my powerpc by building a package with an outdated config.gues an it worked like a charm.

boegel

@ocaisa We should probably add a small README not to the config.guess to explain where it comes from, what is used for, and how it can be updated in the future?

boegel · 2018-09-13T07:28:21Z

easybuild/easyblocks/generic/configuremake.py

-        cmd = "%(preconfigopts)s %(cmd_prefix)s./configure %(prefix_opt)s%(installdir)s %(configopts)s" % {
+        # Avoid using config.guess from the package as it is frequently out of date, use the version shipped with EB
+        build_type, _ = run_cmd('config_guess')
+        build_type_option = '--build=' + build_type.strip()


@ocaisa As indicated by @micaeljtoliveira, this should be config.guess.
Also, please log what the result of config_guess is, and maybe also which config.guess is being (result of which('config.guess')).

Can we detect when --build needs to be used?
I'm a bit worried about the impact this may have on systems where this isn't needed currently...

I don't know enough about Autotools to answer this... @geimer ?

I would expect that it does no harm, config.guess is intended to be updated and so backwards compatible, it's output is the default setting for --build (see https://www.gnu.org/software/autoconf/manual/autoconf-2.61/html_node/Specifying-Names.html).

easybuild/easyblocks/generic/configuremake.py

setup.py

houndci-bot · 2018-09-13T07:33:47Z

easybuild/easyblocks/generic/configuremake.py

+        if build_type is None:
+            build_type, exit_code = run_cmd('config.guess')
+            if exit_code != 0:
+                raise EasyBuildError("config.guess shipped with EB could not determine build type: '%s'",


undefined name 'EasyBuildError'

@ocaisa No need for this, run_cmd check the exit code of the command that was run already by default, and will produce a clear error message when != 0.

So, you can use:

build_type, _ = run_cmd('config.guess')

(_ is often used to signal you're ignoring part of the return value)

boegel · 2018-09-13T07:46:50Z

easybuild/easyblocks/generic/configuremake.py

+        if build_type is None:
+            build_type, exit_code = run_cmd('config.guess')
+            if exit_code != 0:
+                raise EasyBuildError("config.guess shipped with EB could not determine build type: '%s'",


@ocaisa No need for this, run_cmd check the exit code of the command that was run already by default, and will produce a clear error message when != 0.

So, you can use:

build_type, _ = run_cmd('config.guess')

(_ is often used to signal you're ignoring part of the return value)

boegel · 2018-09-13T07:48:22Z

easybuild/easyblocks/generic/configuremake.py

+            build_type = build_type.strip()
+        build_type_option = '--build=' + build_type
+
+        cmd = ("%(preconfigopts)s %(cmd_prefix)s./configure %(prefix_opt)s%(installdir)s %(build_type_option)s "


@ocaisa I like the cmd = ' '.join([...]) option better, there's little point in using a dict here to format the command...

easybuild/easyblocks/generic/configuremake.py

ocaisa · 2018-09-13T08:25:08Z

I think this is ready for primetime, it would be very good if other people could test it though, introducing new code in such a heavily used easyblock makes me nervous @micaeljtoliveira @omula @geimer

ocaisa · 2018-09-13T09:27:51Z

Tested locally and works as expected on this system:

== 2018-09-13 11:18:59,528 run.py:192 INFO running cmd: which config.guess 
== 2018-09-13 11:18:59,536 run.py:192 INFO running cmd: config.guess 
== 2018-09-13 11:18:59,599 run.py:506 INFO cmd "config.guess" exited with exit code 0 and output:
x86_64-pc-linux-gnu

== 2018-09-13 11:18:59,600 configuremake.py:95 INFO /home/alanc/EasyBuild_Git/easybuild-easyblocks/easybuild/scripts/configuremake/config.guess returned a build type x86_64-pc-linux-gnu

micaeljtoliveira · 2018-09-13T10:09:33Z

I'm testing this by rebuilding a few toolchains, both on powerpc and x86.

Found the first problem: zlib uses the ConfigureMake easyblock, but its build system is not autotools based. I guess the --build option should only be passed to packages that have the autotools as a dependency.

…isted as a dep

ocaisa · 2018-09-13T10:34:27Z

Thanks @micaeljtoliveira , I made a fix for that but it also scares me a little. Since this easyblock is inherited all over the place it could easily have an unintended knock-on effect. I wonder is the better option to (also) look in the unpacked sources directory for an existing config.guess and add an additional guard for that?

geimer · 2018-09-13T10:50:46Z

Correctly generated tarballs of Autotools-based projects do not need Autotools as a (build)dependency. Everything should be included in the tarball to run out-of-the-box (unless you need to regenerate the build system due to some change, or require an updated config.guess ;-)). There are of course broken tarballs out there that require the user to run Autotools before configure && make && make install...

W.r.t. looking into the unpacked sources: You know the usual "build systems" of scientific software, right? What if a package uses a homegrown configure script (like zlib), but the tarball also includes all third-party dependencies of which one or more use Autotools?

ocaisa · 2018-09-13T10:58:47Z

@geimer Then the only fix I see for this is the same as the one used by Slack: go in and update every instance of config.guess you find

geimer · 2018-09-14T13:44:47Z

@ocaisa IANAL (I am not a lawyer), but at first glance this seems to comply to the license exception. Maybe it's worth checking how the GPLv3 defines the term 'contains', though. Having said that, I believe the exception was added for configure scripts that actually use config.guess, though this isn't explicitly stated. Thus, maybe adding AC_CANONICAL_BUILD or similar would make sense to be on the safe side?

ocaisa · 2018-09-14T14:00:22Z

"contains" is not defined there. I tried out adding AC_CANONICAL_BUILD but that just leads to more requirements, since it's not explicitly stated I'd prefer not to add more bloat.

micaeljtoliveira · 2018-09-15T12:30:15Z

To add to the discussion about the license issue, it would seem to me that there might actually be no problem at all, as one could argue that the config.guess script and the rest of EasyBuild are separate programs that form an aggregate, instead of being a single program with two parts (see the sections about aggregation and plug-ins in the GPL FAQ). If that is the case, then there is no need for the licenses to be compatible.

ocaisa · 2018-09-17T07:19:45Z

I read that section and I agree with @micaeljtoliveira , the description in the FAQ says:

An “aggregate” consists of a number of separate programs, distributed together on the same CD-ROM or other media. The GPL permits you to create and distribute an aggregate, even when the licenses of the other software are nonfree or GPL-incompatible....pipes, sockets and command-line arguments are communication mechanisms normally used between two separate programs. So when they are used for communication, the modules normally are separate programs.

Since we are forking out and only communicating via the command line, I think this is quite clear that we meet the criteria in the FAQ of being separate programs. If @boegel agrees then I will tidy up the PR and point to the FAQ when discussing the licensing.

boegel · 2018-09-20T18:28:57Z

I'm inclined to agree, but I really don't like this part: Where's the line between two separate programs, and one program with two parts? This is a legal question, which ultimately judges will decide.

We really can't be sure whether this is OK or not without a clear answer from the FSF...

We briefly discussed this during the EasyBuild conf call yesterday (see notes at https://github.com/easybuilders/easybuild/wiki/Conference-call-notes-20180919), where a couple of potential alternatives were suggested:

a) try to download an updated config.guess on the fly rather than packaging it
- could be saved in source path to avoid re-downloading
- failure to download could be ignored, either verbosely (warning message) or silently (log warning), and fallback could be to current behavior
b) add an option in framework to specify the location of an updated config.guess script, and let ConfigureMake use it if and when it's available

I think a) could be a good middle ground... It's pretty much automagic (as long as you're online), and the updated config.guess can easily be "staged in" if needed (e.g. if building software is usually done offline), just like source tarballs can be. And it alleviates the licensing concerns all together...

My approach on it would be to define a constant in ConfigureMake that points to the download location, and it would download the config.guess versioned/checksummed somehow, and then symlink config.guess in $PATH automatically. This allows updating the config.guess in place if the needed occurs.

ocaisa · 2018-09-21T08:23:03Z

@boegel Before I go and implement this, just to clarify

Add to the fetch_step for ConfigureMake to pull config.guess to a fixed location if it's not already there
- use a fixed version of config.guess and create a checksum for it to check against
No need to add to the path, we know exactly the location of the file so can call it directly

boegel · 2018-09-21T08:37:18Z

@ocaisa That sounds like what I had in mind, indeed.

The location should be a generic/ConfigureMake/ subdirectory in the 1st directory listed in the source path. You can use the self.obtain_file method for that in an easyblok, but it would have to be enhanced to allow specifying a particular subdirectory, since now it'll use self.name to determine to location(s) to search + where to download to in case it's missing...

The other option is to use source_paths() from easybuild.tools.config to determine the locations to consider for generic/ConfigureMake/config.guess, and to use download_file from filetools to download to the first entry returned by source_paths() if it's nowhere to be found yet. But then you may end up re-implementing a big chunk of the logic in self.obtain_file, not sure...

I'm not sure how strict we should be w.r.t. the checksum of the config.guess that we pick up, since we probably want to allow using an updated or tweaked config.guess?
So I guess thoroughly logging (location + version + SHA256 checksum of used config.guess) is enough?
But we should download a specific version rather than latest though, otherwise it'll get confusing fast...

In addition, we should consider only running config.guess on platforms other than x86_64 (see get_cpu_architecture() function from easybuild.tools.systemtools...

Running directly rather that updating $PATH makes sense of course.

All of this should probably be handled in a dedicated separate method that just returns the value produced by config.guess, that can then be passed to the --build option?

ocaisa · 2018-09-21T09:04:44Z

I think the option of source_paths() + download_file() + verify_checksum should be easy enough to add to the fetch_step. I would have to add an EB version to the path, so that this gets updated when the framework does. Then I just set a value for self.config_guess and keep the existing logic.

houndci-bot · 2018-09-21T12:08:13Z

easybuild/easyblocks/generic/configuremake.py

+                if verify_checksum(downloaded_path, CONFIG_DOT_GUESS_SHA256):
+                    config_dot_guess_path = downloaded_path
+                    # Add execute permissions
+                    adjust_permissions(downloaded_path, stat.S_IXUSR|stat.S_IXGRP|stat.S_IXOTH, add=True)


missing whitespace around bitwise or shift operator

ocaisa · 2018-09-21T12:13:51Z

@boegel Done and seems to work.

boegel · 2018-09-21T17:28:55Z

@ocaisa That looks a lot better, it's a good middle ground imho.

This way we avoid the licensing concerns, and we allow to easily use an alternate/updated config.guess for experimenting.

I did some additional updates in ocaisa#14, mainly to have better logging of the config.guess that is being used.

more thorough logging on config.guess + code cleanup

…ustom_easyblocks_repo

boegel · 2018-09-21T18:31:28Z

@ocaisa Problem with the broken test is fixed with ocaisa#15

ensure VERSION is defined in easybuild.easyblocks namespace in test_custom_easyblocks_repo

boegel · 2018-09-21T19:16:27Z

Thanks a lot for your efforts on this @ocaisa, and to @geimer and @micaeljtoliveira for the feedback!

Going in to be included in the upcoming EasyBuild v3.7.0...

Sidestep using the config.guess that comes with a package

17d6c76

houndci-bot reviewed Sep 12, 2018

View reviewed changes

ocaisa changed the title ~~Sidestep using the config.guess that comes with a package~~ WIP: Sidestep using the config.guess that comes with a package Sep 12, 2018

ocaisa mentioned this pull request Sep 12, 2018

config.sub and config.guess are frequently out of date for Autotools packages #1276

Closed

boegel requested changes Sep 13, 2018

View reviewed changes

boegel added this to the 3.7.0 milestone Sep 13, 2018

Sidestep using the config.guess that comes with a package

cd69782

boegel added the enhancement label Sep 13, 2018

houndci-bot reviewed Sep 13, 2018

View reviewed changes

boegel requested changes Sep 13, 2018

View reviewed changes

Alan O'Cais added 3 commits September 13, 2018 09:48

Make the changes a bit more resilient

fd6bdb7

Make it clearer why config.guess is there

c47853a

Address comments

ef81748

houndci-bot reviewed Sep 13, 2018

View reviewed changes

easybuild/easyblocks/generic/configuremake.py Outdated Show resolved Hide resolved

easybuild/easyblocks/generic/configuremake.py Outdated Show resolved Hide resolved

Appease the hound

6dcb5c1

ocaisa changed the title ~~WIP: Sidestep using the config.guess that comes with a package~~ Sidestep using the config.guess that comes with a package Sep 13, 2018

ocaisa added 4 commits September 13, 2018 10:06

Update README.md

e120a44

Update README.md

5278326

Update location of config.guess script

5f8fda2

Update README.md

d7115ea

Alan O'Cais added 2 commits September 13, 2018 11:13

Fix bug

80a7a7b

Tidy up

a15fc10

Add a guard to ensure build type is only included when Autotools is l…

2961ea7

…isted as a dep

Update README.md

aa4d649

Download config.guess on the fly (if possible)

adfc4fc

houndci-bot reviewed Sep 21, 2018

View reviewed changes

Appease the hound

7b85a0c

more thorough logging on config.guess + code cleanup

6278426

boegel mentioned this pull request Sep 21, 2018

more thorough logging on config.guess + code cleanup ocaisa/easybuild-easyblocks#14

Merged

ocaisa and others added 2 commits September 21, 2018 19:35

Merge pull request #14 from boegel/config.guess

20a60c1

more thorough logging on config.guess + code cleanup

ensure VERSION is defined in easybuild.easyblocks namespace in test_c…

5cfff14

…ustom_easyblocks_repo

boegel mentioned this pull request Sep 21, 2018

ensure VERSION is defined in easybuild.easyblocks namespace in test_custom_easyblocks_repo ocaisa/easybuild-easyblocks#15

Merged

Merge pull request #15 from boegel/config.guess

f9cb4de

ensure VERSION is defined in easybuild.easyblocks namespace in test_custom_easyblocks_repo

easybuilders deleted a comment from boegelbot Sep 21, 2018

boegel approved these changes Sep 21, 2018

View reviewed changes

boegel merged commit 49a5311 into easybuilders:develop Sep 21, 2018

This was referenced Sep 21, 2018

only read 'configure' script when it's actually there #1522

Merged

Hypre fails to install with EasyBuild v3.7.0 easybuilders/easybuild-easyconfigs#6922

Closed

support bypassing use of system type obtained with recent config.guess in ConfigureMake #1531

Merged

boegel mentioned this pull request Oct 9, 2018

Added an Autoconf easyblock for ppc64le support #1542

Closed

ocaisa deleted the config.guess branch October 27, 2021 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sidestep using the config.guess that comes with a package in ConfigureMake generic easyblock #1506

sidestep using the config.guess that comes with a package in ConfigureMake generic easyblock #1506

ocaisa commented Sep 12, 2018

houndci-bot Sep 12, 2018

micaeljtoliveira commented Sep 13, 2018

boegel left a comment

boegel Sep 13, 2018

ocaisa Sep 13, 2018

houndci-bot Sep 13, 2018

boegel Sep 13, 2018

boegel Sep 13, 2018

boegel Sep 13, 2018

ocaisa commented Sep 13, 2018

ocaisa commented Sep 13, 2018

micaeljtoliveira commented Sep 13, 2018

ocaisa commented Sep 13, 2018

geimer commented Sep 13, 2018

ocaisa commented Sep 13, 2018

geimer commented Sep 14, 2018

ocaisa commented Sep 14, 2018

micaeljtoliveira commented Sep 15, 2018

ocaisa commented Sep 17, 2018

boegel commented Sep 20, 2018

ocaisa commented Sep 21, 2018

boegel commented Sep 21, 2018

ocaisa commented Sep 21, 2018

houndci-bot Sep 21, 2018

ocaisa commented Sep 21, 2018

boegel commented Sep 21, 2018 •

edited

Loading

boegel commented Sep 21, 2018

boegel commented Sep 21, 2018

sidestep using the config.guess that comes with a package in ConfigureMake generic easyblock #1506

sidestep using the config.guess that comes with a package in ConfigureMake generic easyblock #1506

Conversation

ocaisa commented Sep 12, 2018

Choose a reason for hiding this comment

micaeljtoliveira commented Sep 13, 2018

boegel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocaisa commented Sep 13, 2018

ocaisa commented Sep 13, 2018

micaeljtoliveira commented Sep 13, 2018

ocaisa commented Sep 13, 2018

geimer commented Sep 13, 2018

ocaisa commented Sep 13, 2018

geimer commented Sep 14, 2018

ocaisa commented Sep 14, 2018

micaeljtoliveira commented Sep 15, 2018

ocaisa commented Sep 17, 2018

boegel commented Sep 20, 2018

ocaisa commented Sep 21, 2018

boegel commented Sep 21, 2018

ocaisa commented Sep 21, 2018

Choose a reason for hiding this comment

ocaisa commented Sep 21, 2018

boegel commented Sep 21, 2018 • edited Loading

boegel commented Sep 21, 2018

boegel commented Sep 21, 2018

boegel commented Sep 21, 2018 •

edited

Loading