Skip to content

Conversation

@jfgrimm
Copy link
Member

@jfgrimm jfgrimm commented Mar 17, 2022

(created using eb --new-pr)

@casparvl
Copy link
Contributor

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 ;-)

Copy link
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...

# 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
Member

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
Copy link
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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants