Skip to content

Conversation

@Flamefire
Copy link
Contributor

(created using eb --new-pr)

Micket
Micket previously approved these changes Nov 19, 2025
Copy link
Contributor

@Micket Micket left a comment

Choose a reason for hiding this comment

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

lgtm

@Flamefire
Copy link
Contributor Author

Note: This currently doesn't change anything because the only difference of CargoPythonPackage to PythonPackage doesn't matter (yet) for Extensions.
Maybe for the components but there currently would need to be a default easyblock anyway which I haven't seen.
Speaking of it: Should we really unconditionally overwrite the default easyblock in (Cargo)PythonBundle and ignore if it was set by the EC?

@boegel boegel changed the title Make CargoPythonPackage the default class for extensions/components of CargoPythonBundle Make CargoPythonPackage the default class for extensions/components of CargoPythonBundle Nov 19, 2025
@boegel boegel added the change label Nov 19, 2025
@boegel boegel added this to the next release (5.2.0) milestone Nov 19, 2025
"""Specifically use the overloaded variant from Cargo as is populates vendored sources with checksums."""
return Cargo.extract_step(self)

def configure_step(self):
Copy link
Member

Choose a reason for hiding this comment

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

@Flamefire Can you clarify why this change is required?

Also, aren't we missing a return here?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like that's just the change from another PR that is accidentally included here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, the danger if --new-pr. Maybe we marge the other one first so this disappears?

As for the return: We never return anything from configure_step

@Flamefire
Copy link
Contributor Author

This needs a small fix so it works with e.g. pymatgen-2023.12.18-foss-2023a.eb and others

https://github.com/Flamefire/easybuild-easyblocks/blob/f9ab7b091557cd3046faba9992891c50240059e6/easybuild/easyblocks/generic/cargo.py#L240C1-L240C44

this updates sources which in that case is a dictionary and hence fails (updating dict with list errors out)

Proposed fix: Convert sources to a list if it isn't one already in the __init__ call of EasyConfig

@Flamefire Flamefire force-pushed the 20251118151816_new_pr_cargopythonbundle branch from 7964863 to 84b798d Compare December 15, 2025 08:54
@Flamefire
Copy link
Contributor Author

This needs a small fix so it works with e.g. pymatgen-2023.12.18-foss-2023a.eb and others

https://github.com/Flamefire/easybuild-easyblocks/blob/f9ab7b091557cd3046faba9992891c50240059e6/easybuild/easyblocks/generic/cargo.py#L240C1-L240C44

this updates sources which in that case is a dictionary and hence fails (updating dict with list errors out)

Proposed fix: Convert sources to a list if it isn't one already in the __init__ call of EasyConfig

Actually the issue was attempting to add (in this case no) crates to sources for extensions which isn't supported. Fix added
84b798d

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