Skip to content
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

WELD-2589 Support Class-Path manifest attribute when scanning class path for bean archives #1924

Merged
merged 1 commit into from
Jun 28, 2019

Conversation

Vampire
Copy link
Contributor

@Vampire Vampire commented Jun 25, 2019

No description provided.

@WeldJenkins
Copy link

Can one of the admins verify this patch?

@manovotn
Copy link
Contributor

ok to test

Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

Just thinking aloud here; two things that come to my mind:

  • this change means that existing SE apps might break/change behaviour because we will suddenly scan their manifests which means additional archives might be picked up as opposed to previous versions, right?
  • projects/libs might have some 'wrong' entries in the manifest, e.g. repeatedly stating a jar that we otherwise already found and so on - I am not quite sure what will happen then?

@Vampire
Copy link
Contributor Author

Vampire commented Jun 26, 2019

* this change means that existing SE apps might break/change behaviour because we will suddenly scan their manifests which means additional archives might be picked up as opposed to previous versions, right?

Actually yes, this could happen.
But I don't think this will really happen in the wild.
I guess not too many people use the class path scan but provide beans.xml file instead.
And of those that scan the class path, probably only a fraction has class path manifest entries to reference libs that are not added to the classpath in another way.
And for those where it would change the behaviour, I think they didn't use this combination anyway as it wouldn't have worked properly before as the beans only reachable through the class path manifest entries would not have been added before just like it happened for me.
But in theory you are right.

If you really think this will hit people in the wild I would suggest to still have this behaviour by default, as the current behaviour is the more confusing one and provide some Weld-specific system property to turn the class-path manifest entry following off.
But I'm not sure I would have enough time to bake this in right now, I'm away without computer access for next three weeks starting in a few days and I don't know how easy it would be to add this.

* projects/libs might have some 'wrong' entries in the manifest, e.g. repeatedly stating a jar that we otherwise already found and so on - I am not quite sure what will happen then?

Not sure what you mean. If there are wrong entries that do not reference correct files, I already ignore them and if there are libs referenced multiple times, I think I handled this by the endless recursion protection, not scanning a class path attribute referenced lib multiple times.

@manovotn
Copy link
Contributor

...provide some Weld-specific system property to turn the class-path manifest entry following off.

That's what I was considering but I agree that it might be too corner case-ish already.
Might be smarter to just let it in and in case we hear complaints, then add the property.
WDYT @mkouba?

But I'm not sure I would have enough time to bake this in right now, I'm away without computer access for next three weeks starting in a few days and I don't know how easy it would be to add this.

Don't worry, if it comes to that, I'll take it over (although I'll be gone for a while as well).

Not sure what you mean. If there are wrong entries that do not reference correct files, I already ignore them and if there are libs referenced multiple times, I think I handled this by the endless recursion protection, not scanning a class path attribute referenced lib multiple times.

Your code looks defensive enough. I am just being a bit paranoid about what people can do with this. But I haven't found anything that we wouldn't cover here so far; so I think it's good.

Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I'll give Martin time to review as well.

@manovotn manovotn merged commit 31764e3 into weld:master Jun 28, 2019
@manovotn
Copy link
Contributor

@Vampire thanks for contributing! :)

@Vampire Vampire deleted the WELD-2589 branch November 18, 2021 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants