Skip to content

Only consider explicit dependencies in LLVM easyblock#3881

Merged
boegel merged 2 commits intoeasybuilders:developfrom
Flamefire:llvm-ffi
Aug 18, 2025
Merged

Only consider explicit dependencies in LLVM easyblock#3881
boegel merged 2 commits intoeasybuilders:developfrom
Flamefire:llvm-ffi

Conversation

@Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Aug 11, 2025

Avoid considering build dependencies for setting options which may lead to missing libraries at runtime.

Note: I changed self.deps to runtime-dependencies only and set it as early as possible. I can't see where this would change current (intended) behavior based on the code in this easyblock and it was likely intended this way anyway.

No other official easyblock uses self.deps so this should be safe.

See also

Closes #3883

@Flamefire
Copy link
Contributor Author

Flamefire commented Aug 11, 2025

Test report by @Flamefire

Overview of tested easyconfigs (in order)

Build succeeded for 45 out of 47 (18 easyconfigs in total)
n1609 - Linux RHEL 8.9 (Ootpa), x86_64, Intel(R) Xeon(R) Platinum 8470 (sapphirerapids), Python 3.9.18
See https://gist.github.com/Flamefire/58c29a66aa346aa25fc7b14fdf0a8e60 for a full test report.

UPDATE
LLVM 18 passes without sanitizer tests: easybuilders/easybuild-easyconfigs#23636
LLVM 20 passes with additional skips: easybuilders/easybuild-easyconfigs#23637

@Thyre
Copy link
Collaborator

Thyre commented Aug 12, 2025

33aefb3 should probably only be in #3883, right?

@Flamefire
Copy link
Contributor Author

Given the time required to test this I thought it might be good to combine them here at least for testing and rebase afterwards or keep it.

@Thyre
Copy link
Collaborator

Thyre commented Aug 12, 2025

Agreed, that makes sense.

Avoid considering build dependencies for setting options which may lead
to missing libraries at runtime.
@Flamefire
Copy link
Contributor Author

Flamefire commented Aug 12, 2025

I rebased this on the other PR so the commit disappears when merged.

Test with both: easybuilders/easybuild-easyconfigs#23637 (comment)

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 18, 2025

@boegel boegel merged commit f3111d2 into easybuilders:develop Aug 18, 2025
17 checks passed
@Flamefire Flamefire deleted the llvm-ffi branch August 24, 2025 10:20
@Crivella
Copy link
Contributor

Doesen't this risk breaking, or atleast not using a dependency in builds eg on top of EESSI when a dependency might be filtered and than reintroduced by setting EBROOT... through an hook?

@Thyre
Copy link
Collaborator

Thyre commented Aug 25, 2025

Doesen't this risk breaking, or atleast not using a dependency in builds eg on top of EESSI when a dependency might be filtered and than reintroduced by setting EBROOT... through an hook?

Yes.
self.dependencies() ignores filtered dependencies.
The documentation of this function states:

Returns an array of parsed dependencies (after filtering, if requested)

But that is incorrect, as one cannot choose that filtered deps are still returned. This seems to be quite a few years old.
Should we add an optional parameter to self.dependencies() to also allow reporting filtered deps? all_depenencies() also removes hidden ones, so that won't help either.

@Thyre
Copy link
Collaborator

Thyre commented Aug 25, 2025

The problem with also reporting filtered deps is that there might be a legitimate reason why one doesn't want a certain dependency. We might still enable some feature based on that dep though, if we just look at the filtered deps...

Do we need a combination of both (i.e. check for filtered dep and EBROOT...)?

@Flamefire
Copy link
Contributor Author

Do we have an example of this being an issue? Might be worth checking why/how we can run into this in a reasonable use case. Maybe there are other solutions then

@Crivella
Copy link
Contributor

https://github.com/EESSI/software-layer-scripts/blob/99c82b562e243cc6f566e72fbcbbe1a23d568d0c/eb_hooks.py#L816-L834

and

self.general_opts['LLVM_ENABLE_ZLIB'] = 'ON' if 'zlib' in self.deps else 'OFF'

Will probably not work have not tested it yet

@Crivella
Copy link
Contributor

I think the easy solution would be to keep using get_software_root directly and ensure that the dependencies are properly placed when needed.

This is the approach that is taken in all other easyblocks.

@Flamefire
Copy link
Contributor Author

This is the approach that is taken in all other easyblocks.

We are not consistent here. Many easyblocks use the explicit dependencies, e.g.:

So I guess we need a more general solution how to deal with filtered deps. Not sure what the use case is for them but I guess we need something for "replaced" dependencies. I.e. it looks like an "abuse" of this feature in EESSI: Remove a dependency but pretend it is still there. I'd say we need a cleaner solution for that. Isn't that what "external modules" would do too?

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.

4 participants