-
Notifications
You must be signed in to change notification settings - Fork 397
Cherry pick WICKET-7090 to Wicket 9.x (for WICKET-7166) #1278
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
Conversation
In addition to reprocible builds, this should also result in timestamps being set on files in jars.
…plugin for packaging
solomax
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :))
|
I just did a grep on "bundle" in all the pom.xml files and still see some bundle values; Perhaps because these modules no longer exists in wicket 10.x? I will update these as well and add them to this pull request. |
|
@mattrpav Does this affect the OSGi support ? |
It seems I have already asked the same earlier - #758 (comment) |
| </parent> | ||
| <artifactId>wicket-auth-roles</artifactId> | ||
| <packaging>bundle</packaging> | ||
| <packaging>jar</packaging> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this changing from jar? A bundle is a jar. The bundle packaging simply signals for the maven-bundle-plugin to handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same question was asked and answered here: #758 (comment)
Since these changes did not have any negative consequences for the wicket-10 (main) branch, I think it will not be an issue for wicket-9.x either.
Please check the discussion on the wicket 10 pull request (which was merged in 2024) for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference to comment on the #758 -- bundle plugin has supported outputTimestamp for 4+ years. Since v5.1.3. Sounds odd that it wasn't working here -- and how does the timestamp and Maven packaging effect the inclusion of a resource file? Those don't seem related.
ref: (#758 comment)[https://github.com//pull/758#discussion_r2440294774]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it's rather strange that this is affecting the (non-) inclusion of a minified javascript file.
But alas I am not an expert on the inner workings of the various maven resources plugins. As can be seen in my wicket issue I did try to dig into this a bit, but for some reason it simply is not included - even with debug logging there is no indication of anything that might cause this.
@solomax simply experimented a bit, I think, and found that this change affects the inclusion of the min.js file. Then I simply looked at the git blame data to see which commit(s) made that change on main.
Of course I would also like to know what is the root cause here; But it will take time and some knowledge of the maven plugins used. I'm willing to spend some time on this, but do not know how to debug this properly.
After developing some maven plugins I found it's quite hard to debug them in a live project.
|
see: #1279 |
Thanks 👍 For future reference, can you please explain why this fixed it? The fact that the other |
|
@bgooren the bundle plugin (by default) includes resources from the 'source' location of resources (src/main/resources, etc) and needed a hint to pull in resources post-processing (target/classes) |
Hi! I saw that and it makes sense. But one question is still unanswered: it was already pulling in the other min.js files prior to your change, which are generated using the exact same plugin as the missing file, and written to the same location. So why was that one file excluded when the rest was already included? |
|
Will close this, as it was fixed by @mattrpav another way. |
This fixes the missing wicket-ajax-jquery.min.js in the wicket-core artifact.
See https://issues.apache.org/jira/browse/WICKET-7166 for more details.