Skip to content

ensure --review-pr can find dependencies included in PR#3979

Merged
boegel merged 5 commits intoeasybuilders:developfrom
jfgrimm:20220317153211_new_pr_IYWSCmQABI
Mar 29, 2022
Merged

ensure --review-pr can find dependencies included in PR#3979
boegel merged 5 commits intoeasybuilders:developfrom
jfgrimm:20220317153211_new_pr_IYWSCmQABI

Conversation

@jfgrimm
Copy link
Copy Markdown
Member

@jfgrimm jfgrimm commented Mar 17, 2022

(created using eb --new-pr)

@casparvl
Copy link
Copy Markdown
Contributor

casparvl commented Mar 18, 2022

Well, I can confirm that I had a similar issue with --review-pr:

casparl@software1:~$ eb --review-pr 15132
== Temporary log file in case of crash /scratch/casparl/eb-mpZaHN/easybuild-l3LhZo.log
== found valid index for /sw/noarch/Debian10/2021/software/EasyBuild/4.5.3/easybuild/easyconfigs, so using it...
ERROR: Failed to process easyconfig /scratch/casparl/eb-mpZaHN/tmpqnbSC7/l/loompy/loompy-3.0.7-intel-2021b.eb: Failed to determine minimal toolchain for dep numba 0.54.1

For me, it seemed to occur because we use --minimal-toolchains. Running with --disable-minimal-toolchains works:

casparl@software1:~$ eb --review-pr 15132 --disable-minimal-toolchains
== Temporary log file in case of crash /scratch/casparl/eb-_A6yn7/easybuild-yWomfF.log
== found valid index for /sw/noarch/Debian10/2021/software/EasyBuild/4.5.3/easybuild/easyconfigs, so using it...
Comparing loompy-3.0.7-intel-2021b.eb with loompy-3.0.7-intel-2021b.eb
=====
(no diff)
=====
Comparing numba-0.54.1-intel-2021b.eb with numba-0.54.1-intel-2021b.eb
=====
(no diff)
=====

But, running with --minimal-toolchains with this PR also works:

casparl@software1:~$ eb --review-pr 15132 --minimal-toolchains
== Temporary log file in case of crash /scratch/casparl/eb-0CqDCz/easybuild-_fNPPg.log
Comparing loompy-3.0.7-intel-2021b.eb with loompy-3.0.7-intel-2021b.eb
=====
(no diff)
=====
Comparing numba-0.54.1-intel-2021b.eb with numba-0.54.1-intel-2021b.eb
=====
(no diff)
=====

So, in terms of functionality, I'm happy with this PR. Now only thing todo is make the CI happy too ;-)

@jfgrimm jfgrimm requested a review from boegel March 25, 2022 14:56
@jfgrimm jfgrimm mentioned this pull request Mar 28, 2022
Merged
@jfgrimm jfgrimm force-pushed the 20220317153211_new_pr_IYWSCmQABI branch from 1ebbed1 to 7053916 Compare March 28, 2022 15:21
@jfgrimm jfgrimm added this to the next release (4.5.4) milestone Mar 28, 2022
boegel
boegel requested changes Mar 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

@jfgrimm Before making the requested change, please also remove the two commits related to the container test workflow using git push --force, otherwise the commit that was merged via #3985 may get reverted...

Comment thread easybuild/framework/easyconfig/tools.py Outdated
# which are picked up via 'pr_paths' build option in fetch_files_from_pr
pr_paths = None
if from_prs and review_pr:
from_prs.append(review_pr) if review_pr not in from_prs else from_prs
Copy link
Copy Markdown
Member

@boegel boegel Mar 29, 2022

Choose a reason for hiding this comment

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

@jfgrimm Please update to:

if from_prs and review_pr and review_pr not in from_prs:
    from_prs = from_prs + [review_pr]

Two reasons:

  • We shouldn't append to the from_prs list that is passed to alt_easyconfig_paths, since that's a side effect (that change will stick even outside of the scope of the alt_easyconfig_paths call);
  • The if ... else construct here doesn't make sense, in particular the else from_prs part doesn't (since we're not assigning the result of that expression)

@jfgrimm jfgrimm force-pushed the 20220317153211_new_pr_IYWSCmQABI branch from 6ab1ac6 to fa62968 Compare March 29, 2022 08:25
boegel
boegel requested changes Mar 29, 2022
View reviewed changes
Comment thread easybuild/framework/easyconfig/tools.py
boegel
boegel approved these changes Mar 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel boegel merged commit f325c59 into easybuilders:develop Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@boegel boegel boegel approved these changes

Assignees

No one assigned

Labels

bug fix

Projects

None yet

Milestone

4.5.4

Development

Successfully merging this pull request may close these issues.

3 participants

@jfgrimm @casparvl @boegel