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

Update config.guess for all R packages #1949

Merged
merged 8 commits into from
May 18, 2020

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Feb 7, 2020

Factor out the check and obtain config.guess from ConfigureMake into free functions to be able to use them inside the rpackage EB.
Change the workflow for R packages to always extract which is more in line with the regular workflow for other EasyConfigs.
This then allows to search for config.guess files to replace by the downloaded version contained within EasyBuild.

This is an alternate, more complete approach to #1938

I tested some random R based ECs including the underlying R successfully: rgdal-1.4-8-foss-2019b-R-3.6.2.eb ncdf4-1.16.1-foss-2019a-R-3.6.0.eb lavaan-0.6-4.1433-foss-2019a-R-3.6.0.eb

Requires:

easybuild/easyblocks/generic/configuremake.py Outdated Show resolved Hide resolved
easybuild/easyblocks/generic/configuremake.py Outdated Show resolved Hide resolved
easybuild/easyblocks/generic/configuremake.py Outdated Show resolved Hide resolved
easybuild/easyblocks/generic/configuremake.py Outdated Show resolved Hide resolved
easybuild/easyblocks/generic/rpackage.py Show resolved Hide resolved
easybuild/easyblocks/generic/rpackage.py Show resolved Hide resolved
easybuild/easyblocks/generic/rpackage.py Outdated Show resolved Hide resolved
easybuild/easyblocks/generic/rpackage.py Outdated Show resolved Hide resolved
easybuild/easyblocks/generic/rpackage.py Outdated Show resolved Hide resolved
easybuild/easyblocks/generic/rpackage.py Show resolved Hide resolved
@edmondac
Copy link
Contributor

I've tried testing this, but there are too many things in the way. E.g. we need easybuilders/easybuild-easyconfigs#9694 merging. Also older R version have checksum failures (as expected) and I'm not able to go through and fix them right now.

Saying that, it does seem to work for the bits that did build!

easybuild/easyblocks/generic/rpackage.py Outdated Show resolved Hide resolved
@Flamefire
Copy link
Contributor Author

@boegel Updated this with the following changes worth verifying:

  • Use logger (same for check and obtain function)
  • Verify existing file before using it (this avoids a cached config.guess to fail the build if it is an old one, see the removed comment about invalidating the cache which is kinda impossible from the EB side)
  • Verify downloaded file through check_config_guess
  • Honor force_download build option
  • downgrade the print_warning to a log.warning again, but use print_warning in obtain_config_guess when download or verification fails. Otherwise the build output has a few warnings which are not really warnings because the file will be replaced later. I'd actually downgrade the warnings to info in check_config_guess if you agree

Pending approval: Remove arguments of obtain_config_guess. No EasyBlock uses them and Easyblock.obtain_file doesn't have those either. Also the final download location is pretty much opaque to the caller (appended with e.g. EB version) so it doesn't make sense to pass it

@Flamefire
Copy link
Contributor Author

@boegel Test results in easybuilders/easybuild-easyconfigs#9830 show that this works and fixes the issue on Power.

What I like to do before merging this is to remove arguments of obtain_config_guess, see #1949 (comment)
This does not change behavior but makes things easier to maintain

Factor out the check and obtain config.guess from ConfigureMake into free functions
to be able to use them inside the rpackage EB.
Change the workflow for R packages to always extract which is more in line with the
regular workflow for other EasyConfigs.
This then allows to search for config.guess files to replace by the downloaded version
contained within EasyBuild.
…obtain_file

Verify downloaded AND existing file redownloading if required
Honor force_download option
…n_config_guess

- search_source_paths was totally ignored (Bug)
- download_source_path was hard to use as final location was not obvious
- not used by anything in EasyBuild itself

Throw readable error when any argument is passed to make future
maintenance easier and avoid users running into the bug.
@Flamefire
Copy link
Contributor Author

I did the change I proposed now for easier assesment in 8af677b I even found that using the source-* argument didn't work at all which is another point for the change.

The current implementation is to raise an error that those arguments were removed. This caters for external users using that function. The alternative is to simply remove the arguments and let Python handle it by throwing a (IIRC) TypeError on unknown/unexpected arguments. But I guess the current implementation is more in line with EB handling such things.

Not using deprecation on purpose as the implementation contained at least 1 bug. So removing it is safer IMO.

@Flamefire
Copy link
Contributor Author

@boegel ping again. This has been tested successfully in easybuilders/easybuild-easyconfigs#10531 multiple times.

restore 'download_source_path' and 'search_source_paths' argument for ConfigureMake.obtain_config_guess, but deprecate their use
@boegel
Copy link
Member

boegel commented May 18, 2020

I've tested this extensively by installing R-4.0.0-foss-2020a.eb, R-3.6.3-foss-2020a.eb and R-3.4.3-foss-2017b-X11-20171023.eb using this enhanced RPackage + a whole bunch of dependencies usingConfigureMake, no problems encountered, so this is good to go...

@boegel boegel merged commit 0cb9337 into easybuilders:develop May 18, 2020
@Flamefire Flamefire deleted the update_r_config_guess branch May 19, 2020 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants