bndtools / bnd

Bnd/Bndtools. Tooling to build OSGi bundles including Eclipse, Maven, and Gradle plugins.
https://bndtools.org
Other
526 stars 304 forks source link

Repository Browser: Add context menu action "Update Revisions :: To higher MICRO / MINOR / MAJOR revision" #6104

Closed chrisrueger closed 2 months ago

chrisrueger commented 2 months ago

This PR is a suggestion for bnd's MbrCommand e.g. bnd mbr update -s micro but making it available in bndtools' UI.

This adds a context menu when you Right-Click on a MavenBndRepo like this:

image

This allows to increase versions of a full central.mvn maven index file.

Similar functionality already exists if you right-click on a single RepoBundle-Revision. This suggestion here basically does it for the whole repository.

I added a Dry-Run-Option which does not modify the .mvn file, but instead copies the result to the Clipboard. I am not sure if this is a good idea, since it can take some time depending on the size of the repo and the user might not notice it... UX isn't great. I kept it for now, since it helped during debugging, but I am fine with removal too... or suggestions how it could be improved. Better wording suggestions welcome too.

Also: I basically copy pasted and adjusted the code from MbrCommand. There is room for better re-use, but I just wanted to prototype this first.

Dry-Run

The output of the Dry-Run is similar to MbrCommand#_check e.g.:

## Updates available
com.fasterxml.jackson.core:jackson-core:2.16.1 2.16.2
etc.
chrisrueger commented 2 months ago

@pkriens Do you have any thoughts? Would this be a useful feature?

How would you go about the code re-use between MbrCommand.java and this class?

I first thought about reusing MbrCommand directly, but then decided I pull out the stuff first (copy paste). Maybe we should put the code from MbrCommand.java into a util class like I have (Mbr.java) and call that from MbrCommand.java and RepoActions.java.

pkriens commented 2 months ago

I agree that the shared code should be shared. This seems a good place. I would also be ok to make it methods on the MavenBndRepository. But I can go either way.

nice work!

chrisrueger commented 2 months ago

Thanks for the feedback @pkriens

I added some commits trying to share as much code as I could.

One question about something I am not sure: In calculateUpdateRevisions() https://github.com/bndtools/bnd/pull/6104/files#diff-9798d76a25282bc3dff5647b6739f82970d66f20f02645d60e6bad8333fc1b91R86 I added a PrintStream out parameter. But I would rather avoid it. I only did it because previously bnd.out.format(...) was used from MbrCommand. bnd.out seems to be System.out

But there were some other calls to bnd.trace(...) which also seems to call System.out.println() under the hood. The trace() calls I have solved with the Trace logger https://github.com/bndtools/bnd/pull/6104/files#diff-9798d76a25282bc3dff5647b6739f82970d66f20f02645d60e6bad8333fc1b91R36 passed to the constructor.

I was wondering: since both are basically writing to System.out in one way or the other: Could I get rid of the PrintStream out parameter in calculateUpdateRevisions(...) and also use the Trace logger? (logger.trace(...) instead of out.format(...))

pkriens commented 2 months ago

We can use the logger for tracing etc. Maybe it should use Result for the methods instead of outputing information? In general you do not want to do any output in a component like this.

chrisrueger commented 2 months ago

We can use the logger for tracing etc. Maybe it should use Result for the methods instead of outputing information? In general you do not want to do any output in a component like this.

Thanks for the suggestion. unfortunately I did not see how aQute.bnd.result.Result would have helped me to log like before in MbrCommand.

Thus I decided a different approach. I pulled out all logging from the new class MbrUpdater and did the logging manually in MbrCommand. See https://github.com/bndtools/bnd/pull/6104/commits/2d1f949e649f724c2615145f7d013033b07bbd98

The logging output is the same, but the place is obviously a bit different than before, e.g. because now I am logging after I have all the results - while before logging happened on the go during iteration in the loop.

Question: Is this an ok way to check for "updated" versions? https://github.com/bndtools/bnd/pull/6104/commits/2d1f949e649f724c2615145f7d013033b07bbd98#diff-9bfdb19b4bf9085bd6f3d2c890803a16eb8b3fe79b3e4f923dd725e8c4cd1d1aR162-R166

chrisrueger commented 2 months ago

Question: Is this an ok way to check for "updated" versions? 2d1f949#diff-9bfdb19b4bf9085bd6f3d2c890803a16eb8b3fe79b3e4f923dd725e8c4cd1d1aR162-R166

~~Damn, I noticed I misunderstood the previous logging. I thought the previous logging only logged updated versions. But it just logged when the list was not empty (meaning that there was a version returned from maven).~~

- Do you think this is ok? (but different logging behavior, because only version > currentVersion is logged) - or should I change it back to the previous logging behavior, where everything is logged which received a result from maven-central (even though it could be the same version as current version)

Update: Forget the above. I added a new commit. I introduced a new record object in fc97d92513a53710b34dd6871b3b87c87507448a which allows to write the log output the same way as it was before in MbrCommand. The only difference now is that it gets written a bit later after the Map has been built (previously logging happened on the go in the loop).

chrisrueger commented 2 months ago

Some notes about how to test bnd mbr from the cli

cd ~/my-bndtools-workspace

## execute bnd mbr update -d  (-d is dry run)
java -jar ~/Downloads/Dist_Bundles_JDK17_ubuntu-latest/biz/aQute/bnd/biz.aQute.bnd/7.1.0-SNAPSHOT/biz.aQute.bnd-7.1.0-20240503.192058-1.jar bnd mbr update -d 

Expected example output:

com.fasterxml.jackson.dataformat:jackson-dataformat-yaml                             2.16.1 -> 2.17.0
com.fasterxml.jackson.jaxrs:jackson-jaxrs-base                                       2.16.1 -> 2.17.0
pkriens commented 2 months ago

LGTM, want to merge it?

chrisrueger commented 2 months ago

LGTM, want to merge it?

Yes