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

improve handling of start_dir and add tests for cases where either ext_dir or initial start_dir or both are unset or None #4206

Merged
merged 8 commits into from
Feb 7, 2023

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Feb 2, 2023

Python packages may be installed from WHL files which do not get extracted in which case self.start_dir, self.ext_dir are both None. This leads to an unhelpful warning:

WARNING: Provided start dir (None) for extension tensorboard-plugin-wit does not exist:

Check that case and only log a debug message. Also add test for this.

Note that #4196 changed semantics: Prior to this start_dir or ext_dir where used if set and existing resolved relative to the current directory. After that start_dir is resolved relative to ext_dir which is usually the same but does not need to.

Python packages may be installed from WHL files which do not get
extracted in which case `self.start_dir, self.ext_dir` are both `None`.
This leads to an unhelpful warning:
> WARNING: Provided start dir (None) for extension tensorboard-plugin-wit does not exist:
Copy link
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I suggest to update the description of _set_start_dir to reflect the new logic

easybuild/framework/extensioneasyblock.py Outdated Show resolved Hide resolved
@lexming
Copy link
Contributor

lexming commented Feb 6, 2023

As discussed with @Flamefire, these are the target outcomes for _set_start_dir:

start_dir ext_dir=None ext_dir=relative path ext_dir=absolute path
None None error (1) absolute path
empty set to build dir error (1) set to ext_dir
relative absolute path from build dir error (1) absolute path from ext_dir
absolute keep as is error (1) keep as is

1: might be done by some custom easyblock, cannot be handled as we do not know the base dir
2: always remove trailing slashes from absolute paths (except for start_dir='/'). This is needed for easyblocks that already rely on os.path.dirname() and to safely use a future start_dir template.

The result will be that start_dir is always an absolute path except where both ext_dir and start_dir are none (e.g. Python wheels)

Copy link
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lexming
Copy link
Contributor

lexming commented Feb 7, 2023

Test report with this PR and the develop branch of easyblocks and easyconfigs:
https://gist.github.com/lexming/fe9d940570f97d32f90011225f831238

Overview of tested easyconfigs (in order)

  • SUCCESS immunedeconv-2.0.2-foss-2020a-R-4.0.0.eb : RPackage extensions with/without start_dir easyconfig parameter
  • SUCCESS IJulia-1.24.0-Julia-1.8.5.eb : JuliaPackage extensions with/without start_dir easyconfig parameter
  • SUCCESS isoCirc-1.0.4-foss-2020b.eb : PythonPackage extensions with/without start_dir easyconfig parameter
  • SUCCESS IPython-8.5.0-GCCcore-11.3.0.eb : extensions without sources to unpack (PythonPackage with wheels)

@lexming lexming added this to the next release (4.7.1?) milestone Feb 7, 2023
@lexming
Copy link
Contributor

lexming commented Feb 7, 2023

Going in, thanks @Flamefire !

@lexming lexming merged commit 94b97d1 into easybuilders:develop Feb 7, 2023
@lexming lexming changed the title Avoid warning for non-existing start_dir when set to None improve handling of start_dir and add tests for cases where either ext_sir or initial start_dir is set to None Feb 7, 2023
@Flamefire Flamefire deleted the fix-start_dir-msg branch February 7, 2023 14:50
@Flamefire Flamefire changed the title improve handling of start_dir and add tests for cases where either ext_sir or initial start_dir is set to None improve handling of start_dir and add tests for cases where either ext_dir or initial start_dir or both are unset or None Feb 7, 2023
@boegel boegel added the bug fix label Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants