Skip to content

Conversation

@maxim-masterov
Copy link
Contributor

Required to install LLVM-16.x

@migueldiascosta
Copy link
Member

migueldiascosta commented Aug 2, 2023

wondering if it wouldn't be clearer to use something like 'extract_cmd': "mkdir third-party && tar xf %s --strip-components=1 -C third-party" in the easyconfig - if something else changes in the future (e.g. the third-party-%(version)s.src folder name format), the symlink approach will require more version checking in the the easyblock, handling this explicitly in the easyconfig may be preferable (?)

@SebastianAchilles @surak @casparvl, since you're testing the LLVM easyconfig, what's your opinion?

@casparvl
Copy link
Contributor

casparvl commented Aug 2, 2023

I don't really have a strong preference one way or another. I'm inclined to keep these kind of things in the easyblock if they are at least somewhat generic - it keeps the EasyConfigs cleaner. I think this qualifies: there's hopefully a fair chance that they won't change this with every version, at least I don't think they've changed in the the recent major versions before this change. Another advantage is that this gives us the opportunity to raise an error if that folder is not where expected. We could consider rephrasing the error message and giving even more explicit instructions (i.e. look in the source folder, check if the third-party stuff is moved, if so, update the EasyBlock).

Then again, having it in the EasyConfig is indeed a clear indication that it is version dependent (and therefore a 'configurable'). If I find myself in 2 years seeing that they changed it again in the next 3 versions, I would have agreed with you - but there's no way to know for sure.

@boegel boegel changed the title Patch llvm easyblock for LLVM-16 update LLVM easyblock for LLVM v16 Aug 2, 2023
@boegel boegel added the update label Aug 2, 2023
@boegel boegel added this to the 4.x milestone Aug 2, 2023
@boegel boegel changed the title update LLVM easyblock for LLVM v16 update LLVM easyblock for LLVM v16: symlink third-party to third-party-<version>.src Aug 2, 2023
@boegel boegel modified the milestones: 4.x, next release (4.8.1?) Aug 2, 2023
@boegel
Copy link
Member

boegel commented Aug 2, 2023

The easyblock is indeed the right place to deal with this based on what we currently know, I fully agree ("do once and forget").

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

boegel commented Aug 2, 2023

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS LLVM-11.1.0-GCCcore-10.3.0.eb
  • SUCCESS LLVM-15.0.5-GCCcore-12.2.0.eb

Build succeeded for 2 out of 2 (2 easyconfigs in total)
node3106.skitty.os - Linux RHEL 8.6, x86_64, Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz (skylake_avx512), Python 3.6.8
See https://gist.github.com/boegel/a3c46c4a370ff1f462b52817632c50cb for a full test report.

@migueldiascosta
Copy link
Member

fair enough :) to be clear, the reason I have to think that something will still change is that if they had thought this through, we wouldn't have had to change anything, unpacking the tarball would just work. Hopefully they will just make CMake look into third-party-<version>.src instead of third-party such that the symlink won't be necessary but it won't hurt either

@boegel boegel merged commit c683dbe into easybuilders:develop Aug 2, 2023
Comment on lines +121 to +123
cwd = change_dir(self.builddir)
symlink('third-party-%s.src' % self.version, 'third-party')
change_dir(cwd)
Copy link
Contributor

Choose a reason for hiding this comment

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

2 Questions here: Why the symlink and not a move? The name "third-party-version.src" is only because of the filename of the archive, I don't see how it is required to be kept.

2nd: Why the change_dir? symlink('third-party-%s.src' % self.version, os.path.join(self.builddir, 'third-party'), use_abspath_source=False) would be better, wouldn't it?
But I'd just rename it instead.

Copy link
Member

Choose a reason for hiding this comment

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

@Flamefire If you think it's worth changing this, please open an issue or a PR, following up on stuff in merged PRs is a nightmare I don't want to get into :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see: It was C&P from the cmake folder above. Opened PR: #2994

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.

5 participants