-
-
Notifications
You must be signed in to change notification settings - Fork 305
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
Includeresource duplicate strategy to append to existing file when unrolling jar #6326
base: master
Are you sure you want to change the base?
Includeresource duplicate strategy to append to existing file when unrolling jar #6326
Conversation
NOTHING, OVERWRITE, APPEND Signed-off-by: Christoph Rueger <chrisrueger@gmail.com> use SequenceInputStream to concatenate... resources to avoid intermediate Strings Signed-off-by: Christoph Rueger <chrisrueger@gmail.com> handle duplicates for :literal useful for testcases Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
20a2822
to
7c6bb03
Compare
@pkriens one thing I am thinking about and would like to discuss is, wether or not we shoud add a way to enforce a
a line break is not required, since the first file ends with one. One option would be another strategy in addition to APPEND.
|
You are becoming the Elon Musk of bnd :-) Nice productivity. Some comments.
|
Your approach sounds good and more flexible.
I recognize a pattern here. I am always one level too deep 🤣
What about the existing method with the This was the main reason why I have put it here. But since it is public, we have to keep it anyway.
Are you talking plugin as in ? Ok I will try to digest your ideas and see what I can do. |
No, sometimes multiple levels 😎 When I was younger you had "structured design", DeMarco, Michael Jackson, Yourdan, etc. They were talking about coupling and cohesion. Did not get the cohesion until a few years ago but I think it is paramount. I try to write software based on reusable components. So each component must do one thing and one thing only otherwise reuse becomes hard. So I now strife to create classes and methods that do one thing. Very useful in there is not not do control (if/then/switch/case) and actual work in the same method. Anyway, in bnd it is relatively straightforward. Whenever there is choice, it should be in a processor. Things like JAR, Resource, Parameters, etc. are reusable and should just do their work and not take decisions. |
Few questions @pkriens :
I ask because I have things setup so far (not pushed), but not sure how simple or complex the "plugin"-mechanism should be. |
to handle -includeresource expressions like this, as suggested by @pkriens : -includeresource @jar1.jar, @jar2.jar;dup_overwrite:=*, @jar3.jar;dup_skip:="META-INF/services/*,META-INF/MANIFEST.MF" The value of these directives is a list of globs on the paths in the resource. Priority is probably merge (if plugin exists), overwrite, skip. Error/Warning should always be executed even if it matches the other ones. Signed-off-by: Christoph Rueger <chrisrueger@gmail.com> rework to MergeFiles plugin
226d669
to
5df7631
Compare
should avoid re-creating multiple Globs (Pattern.compile()) for each file. Should speed up things e.g. for unrolling jars with lots of files Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@pkriens I pushed the first prototype of this approach. Example: dup_merge AND dup_warning combined:
Warnings are displayed (TODO for me: the message is not correct. the word "overwritten" should not appear when same file is also affected by dup_merge in effect: line break added in META-INF/services files but other files outside META-INF/services will be just appended (without line break). This part is what I would like to discuss: I have created two MergeFiles plugins and iterate over them via
But that does not feel right, because the order of the plugins is important.
|
- instead of iterating of plugins we just check directly which of the two plugins to use, whether we have a META-INF/services file or not (we can get more dynamic in the future if we want, since all is still private) - and add testcases add test for mixed files in META-INF to test that META-INF/services files are merged differently than files in META-INF although they are captured by the same dub_merge glob Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
Meeting notesFrom meeting with @pkriens today. These are just loose notes so I don't forget. I will cleanup later. Duplicate-Strategy:
Example: duplicate:="MERGE,metainfservices" duplicate:="MERGE" - means all merge plugins will be called plugin = tags | enum |
TODO add more testcases for tagging Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
- now we do a warning if duplicate files overwrite eachother and no onduplicate directive is giving. the warning now mentions the directive - finetuned the parsing of the directive which can contain and enum n tags Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@pkriens I think I implemented what we have talked about today. I updated the PR description above One thing though I noticed during the bnd build locally is this output of the new warning we added when duplicate files overwrite eachother and no duplicate strategy is defined. I am not sure this output below is intended. Nor do I know exactly where it is coming from. a) we haven't thought about all side effects of adding a warning (and may remove it again)
|
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
add word 'overwritten' to the default warning. Also use a Constant for the default strategy. Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
Meeting Notes: enum ::= WARN, ERROR, MERGE, SKIP, OVERWRITE MERGE, SKIP, OVERWRITE -> exclusive onduplicate:='MERGE'
onduplicate:='MERGE,metainfservices'
onduplicate:='SKIP,metainfservices'
onduplicate:='metainfservices' (basically no enum, just a tag)
onduplicate:='INVALIDENUM,metainfservices'
onduplicate:='metainfservices,MERGE,SKIP' |
as discussed Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
(only in the default case) Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
I took a look at the code. It is not bad but I think it can be done more readable. The logic when what happens is complex and it is not clear in the code. I came up with the following class (well record) that could help. It is a bit pedantic so you can probably convince me to go with your code but this is the way I would handle the core logic. (The MergeResources is copied for convenience.)
|
You can call the method doDuplicate with the directive content. This gives you back a function you can call when there is a duplicate. Advantage is that the errors are reported early and no unnecessary work is done. |
Thanks a lot @pkriens I started adding your implementation. I think I need to clarify again: What should e.g. happen with a duplicate for:
Should it Basically it could be summarized: I think you implemented a) and I kept that now and modified the testcases accordingly. Also your version did not have the default handling (backwards compatibility, meaning no :onduplicate directive) with the additional warning if resources are not identical. I added that. If the implementation as it is right now is ok and the testcases make sense, then I would say this is ready. |
by @pkriens adjust testcases since this version uses different messages and treats MERGE differently than the previous implementation. Basically now if a MERGE is not supported / possible, then the existing file is kept (is NOT overwritten). Signed-off-by: Christoph Rueger <chrisrueger@gmail.com> fix test Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
85b01b5
to
1153f90
Compare
Closes #6325
Adds a duplicate strategy to
-includeresource
(similar to:flatten
or:rename
)onduplicate:=MERGE
onduplicate:='MERGE,sometag'
onduplicate:='MERGE,metainfservices'
onduplicate:=SKIP
onduplicate:=WARN
onduplicate:=ERROR
e.g.
The
MERGE
strategy:MergeFile
interface)MetaInfServiceMerger.java
which can handle theMETA-INF/services
path and append duplicate service files with a line-breakMERGE
has no effect.Another example using WARN
@pkriens This is the latest based on the discussion today (2024/10/29)