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

build.bnd: avoid resolving default containing ${.} to absolute path #6163

Closed chrisrueger closed 1 week ago

chrisrueger commented 1 week ago

This PR basically changes the default value for a repo index file when adding / editing repos via plugin editor. The previous default was not good, because it contained the ${.} which causes some surprising behavior.

Not sure if this is related to https://github.com/bndtools/bnd/commit/f37f8574b7e887644c036cea073d19d853072159

the ${.} in the default-value caused to expand the value always to an absolute path on the harddrive of the developer editing and saving the file. if you accidentally commit this, then it wil fail on other machines. Let's just define the name, relative to the build.bnd . it can still be changed manually

Steps to reproduce:

  1. Add a MavenBndRepository to the build.bnd a config like below

Notice an index=${.}/central.maven; \ is added (because this was the default value of the index property).

-plugin.6.Centralstaging: \
    aQute.bnd.repository.maven.provider.MavenBndRepository; \
        releaseUrl          ='https://repository.apache.org/content/repositories/staging/';\
        readOnly=true; \
        index=${.}/central.maven; \
        name="Maven Central Staging"
  1. Then save, close build.bnd and reopen again. (in the source view there is still index=${.}/central-staging.maven;)
  2. Click on "Edit" (or double click) on the created MavenBndRepo

Actual result: index=${.}/central-staging.maven; is now expanded to /some/absolute/local/path/central-staging.maven

image

(Sorry the screenshot contains a central-staging.maven, but it is actually a central.maven... I just don't have another screenshot of it).

Expected result I did not want to see the expanded path, but just the value from the source editor.

pkriens commented 1 week ago

hmm. The problem is, is that we resolve ${.} when the file is included. This is necessary because you want it to point to the directory of the file that it contains. You can include files from other directories and then the ${.} changes.

Normally macros are not resolved until the last possible moment. However, this the only special case.

I think we should allow the use of ${.} in the editor? A possible solution might be to replace the ${.} not with the absolute path during the include but with the macro ${.;...absolute path}. We can then resolve it as a normal macro. And editor can then remove the absolute path.

WDYT?

chrisrueger commented 1 week ago

A possible solution might be to replace the ${.} not with the absolute path during the include but with the macro ${.;...absolute path}

Could you point me to the place you have mind where this should happen? I can have a look later.

Update: Is it this? https://github.com/bndtools/bnd/blob/db76001d8f1f45efc6b0c8c404cc56514072d361/biz.aQute.bndlib/src/aQute/bnd/build/model/BndEditModel.java#L1452

And editor can then remove the absolute path.

You mean like a "hackish" removal of the abolute path like

pseudo code: replaceBetween("${.;...absolute path}", ";" , "}")

Just want to confirm, if that is what you have in mind.

chrisrueger commented 1 week ago

@pkriens What about an approach like this:

image
chrisrueger commented 1 week ago

@pkriens I added a change similar as suggested in my Screenshot. The UI now basically says "give me the properties with macros not expanded".

See commit description of f20abf718c7a9574dbafea7e42a7f5e4fa477997 for details.

With this change macros like index=${.}/central.maven; \ are always kept as is without expansion... no matter how often I edit, close, edit, close, reopen build.bnd etc.

Let's discuss.

pkriens commented 1 week ago

men, you're on fire ...

Looks good to me, lets merge and see?