Skip to content

Ignore dyn.shim.excluded.releases when shimplifying #12807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: branch-25.08
Choose a base branch
from

Conversation

razajafri
Copy link
Collaborator

This PR ignores the dyn.shim.excluded.releases when generating shims otherwise, shims don't get generated for files whose root directory is in the shim that is excluded.

fixes #12806

@razajafri razajafri requested a review from a team as a code owner May 29, 2025 20:48
Signed-off-by: Raza Jafri <[email protected]>
@razajafri
Copy link
Collaborator Author

build

value = _get_buildvers(buildvers, "{}/pom.xml".format(multi_module_project_dir), _log, isShimplify=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is hard to follow why this callsite is supposed to be isShimplify=True. Should it rather refer to what this flag is affecting such as ignore_excluded_shims=True or honor_excluded_shims=False with a comment what the difference is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess to me it was clear because if we are calling this from simplify then we don't care about the excluded shims in pom.

I can rename it if it will make it more clear

@sameerz sameerz added the bug Something isn't working label Jun 2, 2025
@razajafri razajafri changed the base branch from branch-25.06 to branch-25.08 June 3, 2025 01:52
@gerashegalov
Copy link
Collaborator

Since this PR is resolving the scenario in #12806 of adding a shim, why is it not restricted to -Dshimplify=true or add/remove?

@razajafri
Copy link
Collaborator Author

Since this PR is resolving the scenario in #12806 of adding a shim, why is it not restricted to -Dshimplify=true or add/remove?

That will produce bad output when running shimplify=false and shimplify.trace i.e. the user will not see the excluded shim as being generated but when he actually runs it, it will modify that directory.

@gerashegalov
Copy link
Collaborator

That will produce bad output when running shimplify=false and shimplify.trace i.e. the user will not see the excluded shim as being generated but when he actually runs it, it will modify that directory.

I am still confused based on the description in #12806 : Is this fix necessary if you don't add or remove shims ?

@razajafri
Copy link
Collaborator Author

That will produce bad output when running shimplify=false and shimplify.trace i.e. the user will not see the excluded shim as being generated but when he actually runs it, it will modify that directory.

I am still confused based on the description in #12806 : Is this fix necessary if you don't add or remove shims ?

That will produce bad output when running shimplify=false and shimplify.trace i.e. the user will not see the excluded shim as being generated but when he actually runs it, it will modify that directory.

I am still confused based on the description in #12806 : Is this fix necessary if you don't add or remove shims ?

I have updated the description PTAL.

Yes, this change is necessary IMO even when just running shimplify=false with trace on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Shimplify.py skips shims that are excluded in pom.xml
3 participants