Skip to content

move verifying of checksums from source to fetch step, to include it with --fetch#4624

Merged
boegel merged 2 commits intoeasybuilders:5.0.xfrom
Flamefire:checksum-step
Sep 9, 2024
Merged

move verifying of checksums from source to fetch step, to include it with --fetch#4624
boegel merged 2 commits intoeasybuilders:5.0.xfrom
Flamefire:checksum-step

Conversation

@Flamefire
Copy link
Copy Markdown
Contributor

After --fetch finishes it is reasonable to expect that the build won't fail due to missing or wrong sources.
However putting the checksum-step into the source-step skips over the verification and one would need to use --stop=source which does a lot more than necessary, e.g. creating build dirs and environment variables.

Move the checksum step into the fetch step and add a test for that.

TODO: We should rename SOURCE_STEP to EXTRACT_STEP, do that here too?

After `--fetch` finishes it is reasonable to expect that the build won't
fail due to missing or wrong sources.
However putting the checksum-step into the source-step skips over the
verification and one would need to use `--stop=source` which does a lot
more than necessary, e.g. creating build dirs and environment
variables.

Move the checksums step into the fetch step and add a test for that.
@boegel boegel changed the title verify checksums with --fetch move verifying of checksums from source to fetch step, to include it with --fetch Sep 7, 2024
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 added the change label Sep 7, 2024
@boegel boegel added this to the 4.9.3 milestone Sep 7, 2024
@boegel
Copy link
Copy Markdown
Member

boegel commented Sep 7, 2024

TODO: We should rename SOURCE_STEP to EXTRACT_STEP, do that here too?

@Flamefire Not sure if we should, source step still makes more sense to me (extracting of sources), but if you want to put that up for discussion, yes let's do so in a separate PR

@boegel boegel modified the milestones: 4.9.3, 5.0 Sep 7, 2024
@Flamefire
Copy link
Copy Markdown
Contributor Author

TODO: We should rename SOURCE_STEP to EXTRACT_STEP, do that here too?

@Flamefire Not sure if we should, source step still makes more sense to me (extracting of sources), but if you want to put that up for discussion, yes let's do so in a separate PR

Well now we have this (extract_step_spec is a new name from me):

extract_step_spec = (SOURCE_STEP, "unpacking", [lambda x: x.extract_step], True)

"source" is a bit ambiguous/too general: It could be related to anything: fetching, verifying, extracting, ... and other steps are named after their called methods too.

The change is for a separate PR but IMO we can discuss/decide this here already where it becomes an "issue" due to the new logic.

@boegel boegel added the EasyBuild-5.0 EasyBuild 5.0 label Sep 9, 2024
@boegel boegel merged commit 8569344 into easybuilders:5.0.x Sep 9, 2024
@boegel
Copy link
Copy Markdown
Member

boegel commented Sep 9, 2024

TODO: We should rename SOURCE_STEP to EXTRACT_STEP, do that here too?

@Flamefire Not sure if we should, source step still makes more sense to me (extracting of sources), but if you want to put that up for discussion, yes let's do so in a separate PR

Well now we have this (extract_step_spec is a new name from me):

extract_step_spec = (SOURCE_STEP, "unpacking", [lambda x: x.extract_step], True)

"source" is a bit ambiguous/too general: It could be related to anything: fetching, verifying, extracting, ... and other steps are named after their called methods too.

The change is for a separate PR but IMO we can discuss/decide this here already where it becomes an "issue" due to the new logic.

We use source because that step was about verifying checksums of sources (+ patches) + unpacking the sources.
Now it's only about unpacking, but changing the publicly facing name of a step is quite intrusive, many people are used to using eb --stop source.

If we want come up with a way of deprecating the use of source, and come up with a non-ambiguous alternative (extract is perhaps too vague), then I'm happy to follow up in another PR

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.

2 participants