Skip to content

fix support for specifying multiple PRs to --from-pr#3707

Merged
boegel merged 2 commits intoeasybuilders:developfrom
migueldiascosta:fix_from_multiple_prs
May 27, 2021
Merged

fix support for specifying multiple PRs to --from-pr#3707
boegel merged 2 commits intoeasybuilders:developfrom
migueldiascosta:fix_from_multiple_prs

Conversation

@migueldiascosta
Copy link
Copy Markdown
Member

@migueldiascosta migueldiascosta commented May 27, 2021 •
edited by boegel
Loading

edit (by @boegel): #3605 was merged while it actually broke the tests for --from-pr, but that went unnoticed until after the PR was merged because tests that require a GitHub token are skipped in PRs (to avoid that the token can be leaked via a malicious PR).

======================================================================
ERROR: test_from_pr (test.framework.options.CommandLineOptionsTest)
Test fetching easyconfigs from a PR.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/runner/2b4db472a006f643ca68e6fd40dc7586a07825d4/lib/python2.7/site-packages/test/framework/options.py", line 1750, in test_from_pr
    outtxt = self.eb_main(args, logfile=dummylogfn, raise_error=True)
  File "/tmp/runner/2b4db472a006f643ca68e6fd40dc7586a07825d4/lib/python2.7/site-packages/test/framework/utilities.py", line 325, in eb_main
    raise myerr
EasyBuildError: u'Trying to symlink /tmp/eb-efsKZA/eb-TA0m_n/eb-j0Hwfp/eb-r1mV11/tmpIFksg8/eb-wHCXVc/tmpzNvNA1/easybuilders/easybuild-easyconfigs-develop/easybuild/easyconfigs/v to /tmp/eb-efsKZA/eb-TA0m_n/eb-j0Hwfp/eb-r1mV11/tmpIFksg8/eb-wHCXVc/files_pr12150_12366/v, but the symlink already exists and points to /tmp/eb-efsKZA/eb-TA0m_n/eb-j0Hwfp/eb-r1mV11/tmpIFksg8/eb-wHCXVc/tmplW3KxT/easybuilders/easybuild-easyconfigs-develop/easybuild/easyconfigs/v.'

@migueldiascosta migueldiascosta added this to the 4.4.0 milestone May 27, 2021
@migueldiascosta migueldiascosta requested a review from boegel May 27, 2021 12:49
@akesandgren
Copy link
Copy Markdown
Contributor

akesandgren commented May 27, 2021

Why is test/framework/easyconfigs/test_ecs/o/OpenBLAS/OpenBLAS-0.3.1-GCC-7.3.0-2.30.eb included in this PR?

@boegel
Copy link
Copy Markdown
Member

boegel commented May 27, 2021

@migueldiascosta The fact that you need to add the OpenBLAS easyconfig to fix the test is a sign of trouble: that should not be needed, because --from-pr should set things up such that not only the files in the PR are available, but also everything else in develop which includes OpenBLAS-0.3.1-GCC-7.3.0-2.30.eb (even though it's not a part of easybuilders/easybuild-easyconfigs#6424)...

boegel
boegel reviewed May 27, 2021
View reviewed changes
Comment thread easybuild/framework/easyconfig/tools.py Outdated
tmpdir = tempfile.mkdtemp()
for pr in from_pr_list:
pr_files.extend(fetch_easyconfigs_from_pr(pr))
pr_files.extend(fetch_easyconfigs_from_pr(pr, path=os.path.join(tmpdir, 'files_pr%s' % pr)))
Copy link
Copy Markdown
Member

@boegel boegel May 27, 2021

Choose a reason for hiding this comment

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

Just changing this here is not enough, and even incorrect (that's why you found yourself having to add the OpenBLAS easyconfig to the tests to make things work).

There's a hidden shared path between alt_easyconfig_paths (which was tweaked in #3605 to take into account multiple PR #s, so you get a path like /.../files_pr123_456 when using --from-pr 123,456), which then gets set as the value for the pr_path build option. This is then used in fetch_files_from_pr to determine the default value for the easyconfigs being pulled from a PR (which you're overriding here).

So if we spread easyconfigs from PRs over multiple paths, we have to take that into account in alt_easyconfig_paths too, or we ensure that all those paths get injected into the robot search path correctly (which is done via the result produced by det_robot_path in set_up_configuration)...

@boegel boegel changed the title fetch easyconfigs to unique dir per PR fix support for specifying multiple PRs to --from-pr May 27, 2021
@easybuilders easybuilders deleted a comment from boegelbot May 27, 2021
@boegel
Copy link
Copy Markdown
Member

boegel commented May 27, 2021

I've fixed the issue by downloading the files of each PR into a separate directory, and adding all those to the robot search path. That way there can't be any clashes between the files from multiple PRs, and order is retained.

It's not perfect, because for files that already exist in develop will only be picked up from the first PR, but at least it's not utterly broken now by trying to symlink into already existing paths.

Re-engineering this to correctly "merge" them multiple PRs in a single view and covering all possible cases of clashes between the multiple PRs won't be easy...

@boegel
Copy link
Copy Markdown
Member

boegel commented May 27, 2021

I'll go ahead and merge this, despite having touched quite a bit of stuff here, to avoid that delays the v4.4.0 release any further.

@migueldiascosta If there's anything here that concerns you or that you're not happy with, do let me know!

@boegel boegel merged commit 375232b into easybuilders:develop May 27, 2021
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.4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@migueldiascosta @akesandgren @boegel