Migrate to latest commons-logging (currently 1.3.3) #3137

HannesWell · 2024-08-09T23:11:36Z

Adapt to the new Bundle-SymbolicName in target-files, the additional.bundles required for development and defined in build.properties and adjust the templates to generate the new name.

Should fix #3127 for Xtext.

cdietrich · 2024-08-10T07:54:42Z

there is still the copy-dependencies the wizard does in some configs that mentions the old name in excludeArtifactIds (IdeProjectDescriptor.xtend) + AbstractPluginProjectCreator also mentions it.
we also need to check into the failing build.

as my team at work in decimated over the next weeks and after that i am on vacation myself i am not sure when we should bring it in. either immediately or right after the release

cdietrich · 2024-08-10T08:09:44Z

...eclipse.xtext.xtext.wizard/src/org/eclipse/xtext/xtext/wizard/RuntimeProjectDescriptor.xtend

 	override setEnabled(boolean enabled) {
 		if (!enabled)
 			throw new IllegalArgumentException("The runtime project is always enabled")
 		super.enabled = enabled
 	}
-


can you undo the ws changes.

(you can run cliwizardintegrationtest as java main to update test expectations

I cannot reproduce the test failure locally. And if I read the error message correct it looks like there is just an additional space between square brackets but I don't understand how this change influences that.
But I pushed an update where I removed the whitepace as requested.

... While typing this and thinking again. These whitespace changes probably caused the test-failures so they are hopefully fixed now.

HannesWell · 2024-08-10T08:41:42Z

there is still the copy-dependencies the wizard does in some configs that mentions the old name in excludeArtifactIds (IdeProjectDescriptor.xtend)

Yes I saw these (templates for) pom.xml files, but since it is a configuration for the maven-dependency-plugin and the element is named excludeArtifactIds I'm not sure if the current name is even correct?
To me it looks like that should be the Maven artifactId which would be commons-logging:
https://mvnrepository.com/artifact/commons-logging/commons-logging

Or does this mojo also consider the PDE dependencies injected by Tycho?
In general I have to admit that I have not yet understood why the dependencies are copied at all.

AbstractPluginProjectCreator also mentions it

That's an import-package header value (at least the method name implies it:
https://github.com/eclipse/xtext/blob/2bce8b6c62b1da7c57d81cff6e699eedb19d90a9/org.eclipse.xtext.ui/src/org/eclipse/xtext/ui/wizard/AbstractPluginProjectCreator.java#L58-L62

we also need to check into the failing build.

I'm looking into this.

as my team at work in decimated over the next weeks and after that i am on vacation myself i am not sure when we should bring it in. either immediately or right after the release

I think it also depends on how fast MWE2 is adjusted, but the commit on your branch looks good, I just have a one or two things that are missing. Do you want to create a PR in MWE2 for that? Then I can add my remarks.

Or if we have something like suggested in eclipse-mwe/mwe#296 (comment), we can maybe completely remove the additional.bundles entries in the build.properties.

cdietrich · 2024-08-10T08:44:21Z

	«ENDIF»
						«IF config.languageServer!=LanguageServer.NONE»
							«IF isEclipsePluginProject»

so this seems to be if you select tycho and language server

cdietrich · 2024-08-10T08:45:29Z

about

additional.bundles

this wont work if you just create a project without any tycho.

unfortunately there is no
additional.imports ....

HannesWell · 2024-08-10T09:34:24Z

as my team at work in decimated over the next weeks and after that i am on vacation myself i am not sure when we should bring it in. either immediately or right after the release

Thinking about this again. Since I assume that mwe workflows will work with the old an new version of commons-logging (since it is just a plain old flat-classpath java app) I think this change is quite save. We don't even have to await the corresponding change for mwe2 since one could already now change existing the 'devlopment-bundles' of an xtext project and the workflow should still run.

So once the execludedArtifacts in pom(templates) are correct, I'm for submitting this now.

HannesWell · 2024-08-10T09:35:52Z

@cdietrich regarding the corresponding change in MWE2 are you fine if I take your commit to a separate branch add my changes and create a PR from it?

cdietrich · 2024-08-10T09:37:00Z

Yes. What are the additions?

cdietrich · 2024-08-10T09:42:04Z

The idea would be

merge mwe
Mwe milestone m2
Merge this
Create mwe bump to m2
Merge it
Check if running workflow in Xtext dev env works
Update reference projects
Merge them
Wait for fallout and fix

HannesWell · 2024-08-10T09:46:16Z

Yes. What are the additions?

See eclipse-mwe/mwe#298.

The idea would be

As you prefer. When is the mwe m2 planned? Couldn't we test it with a nightly build? This would make the cycle shorter.

cdietrich · 2024-08-10T09:48:22Z

I can do mwe m2 it later today when home from hiking (there is no planned date as so far no changes )

cdietrich · 2024-08-10T09:49:44Z

Maybe you can build a branch of Xtext/xtext-reference-projeczs with your mwe and Xtext build results but I fear doing all the adjustments will be a lot

cdietrich · 2024-08-10T09:51:04Z

Do you have some capacity to investigate fallout if there is any contra to my expectations

HannesWell · 2024-08-10T10:00:53Z

Maybe you can build a branch of Xtext/xtext-reference-projeczs with your mwe and Xtext build results but I fear doing all the adjustments will be a lot

Which project's are you exactly referring to when you say Xtext/xtext-reference-projeczs ? The examples in this repository like org.eclipse.xtext.example.arithmetics or org.eclipse.xtext.example.domainmodel?

Do you have some capacity to investigate fallout if there is any contra to my expectations

It depends on when the fallout will happen and what the time-frame is to fix it.
This weekend I would have some more time, next week a bit less, but there should still be spots in the evening.

I can do mwe m2 it later today when home from hiking (there is no planned date as so far no changes )

Enjoy your hike :)

cdietrich · 2024-08-10T10:03:22Z

Github.com/xtext/xtext-reference-projects

there is launch to regenerate wizard projecte
Domain model needs manual copy paste
Integratintests/ (swt) might need target adoption

cdietrich · 2024-08-10T11:09:49Z

@HannesWell i am back home.
if you are d'accord i'd propose to merge the MWE and then build a MWE milestone

HannesWell · 2024-08-10T11:17:40Z

Yes I'm fine with that.
I have limited availability in the next 2-3 hours but will be fully available in the afternoon/evening.

cdietrich · 2024-08-10T11:45:43Z

#3141
xtext/xtext-reference-projects#434

cdietrich · 2024-08-10T13:28:51Z

Mwe bump build is still running. Will merge once it is through

cdietrich · 2024-08-10T13:58:19Z

@HannesWell mwe bump is in. Can you please rebase

HannesWell · 2024-08-10T14:21:49Z

@HannesWell mwe bump is in. Can you please rebase

Done.

Thanks for the milestone and adjustments!

cdietrich · 2024-08-10T14:23:47Z

ok. will create a branch for reference projects

cdietrich · 2024-08-10T14:27:52Z

Regarding the IdeProjectDescriptor.
maybe "viel hilft viel" and adding both might be an option.
i also see this in domainmodel.ide

cdietrich · 2024-08-10T14:33:09Z

xtext/xtext-reference-projects#435

cdietrich · 2024-08-10T14:38:13Z

hmmmm https://github.com/xtext/xtext-reference-projects/actions/runs/10332344902/job/28603464575

any idea?

do we have to add it to extra requirements?
shouldnt tycho resolve it anyway?

https://github.com/xtext/xtext-reference-projects/blob/5718762516bc29e1af3374f4de322bc806cd9407/domainmodel/2.36.0/org.eclipse.xtext.example.domainmodel.releng/pom.xml#L110
does not help neither

why does the

    <location includeAllPlatforms="false" includeConfigurePhase="false" includeMode="planner" includeSource="true" type="InstallableUnit">
      <!--
        Just mention the p2 repo so that additional requirements with ranges specified in
        features and bundles are taken from Orbit (e.g., objectweb.asm)
      -->
      <repository location="https://download.eclipse.org/tools/orbit/simrel/orbit-aggregation/2024-09"/>
    </location>
``` no longer work ...

cdietrich · 2024-08-10T14:58:27Z

https://github.com/xtext/xtext-reference-projects/blob/25cdadea4af21df72378ac3783f59039ff223f08/domainmodel/2.36.0/org.eclipse.xtext.example.domainmodel.releng/tp/domainmodel.target#L28

seems to help but of course is very meh...

HannesWell · 2024-08-10T15:16:04Z

Sorry installing my 'Balcony power plant' took longer than expected, but I'm back now.

do we have to add it to extra requirements? shouldnt tycho resolve it anyway?

https://github.com/xtext/xtext-reference-projects/blob/5718762516bc29e1af3374f4de322bc806cd9407/domainmodel/2.36.0/org.eclipse.xtext.example.domainmodel.releng/pom.xml#L110 does not help neither

That should not be necessary at all. Tycho should resolve the additional.bundles as dependencies too.

Are you sure the target-platform in use contains the new commons-logging artifact?

cdietrich · 2024-08-10T15:24:53Z

it does but not explicitely. we removed it (explicit listings from domain model) with
eclipse/xtext-eclipse#1686
eclipse/xtext-eclipse#1994
eclipse/xtext-eclipse#1687

now using planner instead of slicer

tycho bug?

cdietrich · 2024-08-10T15:25:27Z

https://github.com/xtext/xtext-reference-projects/blob/5718762516bc29e1af3374f4de322bc806cd9407/domainmodel/2.36.0/org.eclipse.xtext.example.domainmodel.releng/pom.xml#L110
would partially revert that

HannesWell · 2024-08-10T15:30:13Z

Ok the domainmodel.target contains the new org.apache.commons.commons-logging at the repos

The problem is, no IU listed in the TP seems to require the new version, but something pulls in the old org.apache.commons.logging. Probably some older feature or a bundle using require-bundle or a tight import-package version range? I have not yet investigated in detail.

So to solve this you can just add the corresponding IU to one of the locations. E.g.

      <unit id="org.apache.commons.commons-logging"/>
      <repository location="https://download.eclipse.org/releases/2024-09"/>

A separate location is not necessary.

tycho bug?

With the explanation above, I don't think so. Problem is that in general, Tycho, like PDE only resolves requirements of IUs listed in the target, but not extra requirements of the projects being built. Oomph targlets are different in that sense. You only have to specify a list of repos and get the full transitive dependency closure in your TP of the Plug-in projects in your workspace.

HannesWell · 2024-08-10T15:32:10Z

In general you can also use multiple repositories in one location. Under the hood PDE/Tycho uses the repos of all locations when resolving IUs using the planner. It can be useful if you view the content of your TP and sort it by location, but for the overall TP content there is no difference if you have one location with all your IUs and repo-URLs listed or one location per repo.

cdietrich · 2024-08-10T15:33:17Z

ok can you adapt your pr?
with org.apache.commons.logging have no idea what could pull this.
dont see it in plugin dependencies view

HannesWell · 2024-08-10T15:38:43Z

with org.apache.commons.logging have no idea what could pull this.
dont see it in plugin dependencies view

Looks like its apache-httpclient

ok can you adapt your pr?

Sorry I don't understand what has to be adapted in this PR?
I assumed we are talking about yours xtext/xtext-reference-projects#435?

LorenzoBettini · 2024-08-10T15:48:05Z

why does the

    <location includeAllPlatforms="false" includeConfigurePhase="false" includeMode="planner" includeSource="true" type="InstallableUnit">
      <!--
        Just mention the p2 repo so that additional requirements with ranges specified in
        features and bundles are taken from Orbit (e.g., objectweb.asm)
      -->
      <repository location="https://download.eclipse.org/tools/orbit/simrel/orbit-aggregation/2024-09"/>
    </location>
``` no longer work ...

That breaks if Orbit provides different bundles like commons logging with different names and transitive dependencies kick in, like in this case.

I added that part some time ago; I see you already added the explicit dependency on the new commons-logging; maybe the comment has to be tweaked accordingly.

cdietrich · 2024-08-10T15:50:13Z

@HannesWell
org.eclipse.xtext.xtext.ui.examples/projects/domainmodel/org.eclipse.xtext.example.domainmodel.releng/tp/domainmodel.target

cdietrich · 2024-08-10T15:51:05Z

and what depends on jgit?

Adapt to the new Bundle-SymbolicName Fixes eclipse-xtext#3127 for Xtext

cdietrich · 2024-08-10T15:58:47Z

so what is left is IdeProjectDescriptor

HannesWell · 2024-08-10T16:04:15Z

@HannesWell org.eclipse.xtext.xtext.ui.examples/projects/domainmodel/org.eclipse.xtext.example.domainmodel.releng/tp/domainmodel.target

Indeed, thanks.

and what depends on jgit?

In Xtext I believe nothing. My other guess is ECF on which P2 depends. I could invesitgate further, but in the end I think it is sufficient to assume that something requires the old commons.logging but nothing in the TP requires the new one.

so what is left is IdeProjectDescriptor

Yes. The maven-dependency-plugin config.
I'll have a look at this.

Btw. the build of the previous state succeeded.

why does the
    <location includeAllPlatforms="false" includeConfigurePhase="false" includeMode="planner" includeSource="true" type="InstallableUnit">
      
      <repository location="https://download.eclipse.org/tools/orbit/simrel/orbit-aggregation/2024-09"/>
    </location>
``` no longer work ...
That breaks if Orbit provides different bundles like commons logging with different names and transitive dependencies kick in, like in this case.

I added that part some time ago; I see you already added the explicit dependency on the new commons-logging; maybe the comment has to be tweaked accordingly.

I think this is still correct. P2/PDE/Tycho resolves the latest possible version of all requirements. And as long as something requires the 'old' commons-logging, either because of using the old-name and Require-Bundle or Import-Package with a version below 1.3 that old version is resolved (too). It can also be that both versions are resolved. If one cannot satisfies all requirements.

It could of course also happen that the old one can serve all requirements and thus the new one is not resolved/used.

cdietrich · 2024-08-10T18:24:43Z

jar tf org.xtext.example.mydsl.ide-1.0.0-SNAPSHOT-ls.jar | grep logging
shows common logging is included in the uber jar

no idea how this works

=> propose to add the new bundle name and or commons-logging
and also
<exclude>*:org.xtext.example.mydsl.ide-commons-logging*</exclude>
or something similar (seems not to be enhough)

LorenzoBettini · 2024-08-10T18:34:36Z

jar tf org.xtext.example.mydsl.ide-1.0.0-SNAPSHOT-ls.jar | grep logging shows common logging is included in the uber jar

no idea how this works

=> propose to add the new bundle name and or commons-logging and also <exclude>*:org.xtext.example.mydsl.ide-commons-logging*</exclude> or something similar (seems not to be enhough)

I seem to understand that the exclusion has not been updated so the jar is in the uber jar for the moment.

cdietrich · 2024-08-10T18:34:53Z

guess we need

<exclude>*:org.xtext.example.mydsl.ide-commons-logging*</exclude>
							<exclude>*:*commons-logging*</exclude>

and

<excludeArtifactIds>
								com.ibm.icu,
								org.apache.ant,
								org.apache.commons.lang,
								org.apache.commons.logging,
								org.apache.commons.commons-logging,
								commons-logging,

did not check the non shade variant yet

HannesWell · 2024-08-10T22:17:03Z

The configuration already seems to be broken, to fix it I created a separate PR:

Fix exclusion for LanguageServer uber jar assembly #3143

You can notice the problem when looking into the <dsl-project>.ide/target/libs folder into which the dependencies are copied.
It still happens that e.g. commons.logging ends up in the uber-jar, even if it is not in the libs folder. This is probably because the shade plugin finds it as a 'Maven'-Depenendency and it includes all of them in the uber-jar.
I assume the exclusion is only to avoid duplicates.

cdietrich · 2024-08-11T05:45:44Z

thx @HannesWell

cdietrich reviewed Aug 10, 2024

View reviewed changes

HannesWell force-pushed the migrate-to-commons-logging-1.3 branch from c5be99b to 00ba972 Compare August 10, 2024 09:24

HannesWell force-pushed the migrate-to-commons-logging-1.3 branch from 00ba972 to 454df49 Compare August 10, 2024 14:21

Migrate to latest commons-logging (currently 1.3.3)

5eef4f7

Adapt to the new Bundle-SymbolicName Fixes eclipse-xtext#3127 for Xtext

HannesWell force-pushed the migrate-to-commons-logging-1.3 branch from 454df49 to 5eef4f7 Compare August 10, 2024 15:55

cdietrich approved these changes Aug 11, 2024

View reviewed changes

cdietrich merged commit 7e5bfc8 into eclipse-xtext:main Aug 11, 2024
7 checks passed

HannesWell deleted the migrate-to-commons-logging-1.3 branch August 11, 2024 06:29

HannesWell mentioned this pull request Aug 11, 2024

Fix exclusion for LanguageServer uber jar assembly #3143

Closed

cdietrich mentioned this pull request Aug 11, 2024

Investigate if there is a better solution for the Wizard Eclipse + LSP Generation #3146

Open

Migrate to latest commons-logging (currently 1.3.3) #3137

Migrate to latest commons-logging (currently 1.3.3) #3137

Conversation

HannesWell commented Aug 9, 2024

cdietrich commented Aug 10, 2024 • edited Loading

cdietrich Aug 10, 2024

Choose a reason for hiding this comment

cdietrich Aug 10, 2024

Choose a reason for hiding this comment

HannesWell Aug 10, 2024

Choose a reason for hiding this comment

HannesWell commented Aug 10, 2024

cdietrich commented Aug 10, 2024

cdietrich commented Aug 10, 2024

HannesWell commented Aug 10, 2024

HannesWell commented Aug 10, 2024

cdietrich commented Aug 10, 2024

cdietrich commented Aug 10, 2024

HannesWell commented Aug 10, 2024

cdietrich commented Aug 10, 2024

cdietrich commented Aug 10, 2024

cdietrich commented Aug 10, 2024

HannesWell commented Aug 10, 2024

cdietrich commented Aug 10, 2024

cdietrich commented Aug 10, 2024

HannesWell commented Aug 10, 2024

cdietrich commented Aug 10, 2024 • edited Loading

cdietrich commented Aug 10, 2024

cdietrich commented Aug 10, 2024

HannesWell commented Aug 10, 2024

cdietrich commented Aug 10, 2024

cdietrich commented Aug 10, 2024

cdietrich commented Aug 10, 2024

cdietrich commented Aug 10, 2024 • edited Loading

cdietrich commented Aug 10, 2024

HannesWell commented Aug 10, 2024

cdietrich commented Aug 10, 2024 • edited Loading

cdietrich commented Aug 10, 2024

HannesWell commented Aug 10, 2024

HannesWell commented Aug 10, 2024

cdietrich commented Aug 10, 2024

HannesWell commented Aug 10, 2024

LorenzoBettini commented Aug 10, 2024

cdietrich commented Aug 10, 2024

cdietrich commented Aug 10, 2024

cdietrich commented Aug 10, 2024

HannesWell commented Aug 10, 2024

cdietrich commented Aug 10, 2024 • edited Loading

LorenzoBettini commented Aug 10, 2024

cdietrich commented Aug 10, 2024

HannesWell commented Aug 10, 2024

cdietrich commented Aug 11, 2024

cdietrich commented Aug 10, 2024 •

edited

Loading

cdietrich commented Aug 10, 2024 •

edited

Loading

cdietrich commented Aug 10, 2024 •

edited

Loading

cdietrich commented Aug 10, 2024 •

edited

Loading

cdietrich commented Aug 10, 2024 •

edited

Loading