Skip to content
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

Remove SLF4J Plugin from o.e.passage.lic.oshi feature #1127

Merged
merged 2 commits into from
Nov 14, 2022

Conversation

HannesWell
Copy link
Contributor

If a Feature includes a Plugin, it usually includes it with a specific name and the version that was in the TP when the feature was build. This prevents consumers from using a slf4j bundle with different symbolic-name or different version in their TP or product.

This is part of eclipse-platform/eclipse.platform.releng.aggregator#588

@codecov
Copy link

codecov bot commented Sep 28, 2022

Codecov Report

Merging #1127 (7d208fa) into master (70266f3) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1127   +/-   ##
=========================================
  Coverage     33.58%   33.58%           
  Complexity      359      359           
=========================================
  Files          1161     1161           
  Lines         25738    25738           
  Branches       1592     1592           
=========================================
  Hits           8644     8644           
  Misses        16574    16574           
  Partials        520      520           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ruspl-afed
Copy link
Contributor

This bundle was added to guarantee that Passage update site is self-sufficient. Who will supply this bundle if we remove it from this feature @HannesWell ?

@HannesWell
Copy link
Contributor Author

This bundle was added to guarantee that Passage update site is self-sufficient. Who will supply this bundle if we remove it from this feature @HannesWell ?

In order to ensure that a p2-repo is self-contained I recommend to use includeAllDependencies of the tycho-p2-repository.
To SimRel SLF4J is for example supplied by Eclipse-Platform or Eclipse-M2E, but we are about to change to the variant from Maven-Central.

The important part about the removal from the feature is, that Plugins included in feature are forced with that exact bundle-name and version into Products that install that feature. If it is not included Product-builders can overwrite the 'default' for example from the SimRel repo that will be fetched by P2 with a newer version. Nevertheless P2 will always ensure that there is one version available to satisfy all Plugin requirements.

@ruspl-afed
Copy link
Contributor

I recommend to use includeAllDependencies

this link does not work.

And, if you suggest this solution, shouldn't it be a part of this PR to satisfy self-containment requirement for SimRel contributions?

@HannesWell HannesWell force-pushed the removeSLF4JFromFeature branch 2 times, most recently from 977ac07 to 803278f Compare October 3, 2022 20:07
@HannesWell
Copy link
Contributor Author

I recommend to use includeAllDependencies

this link does not work.

Indeed, the link changed with the Tycho 3.0.0 since the doc has a version since then.
This is the new one, for Tycho 3.0, but it is the same for older releases:
https://tycho.eclipseprojects.io/doc/3.0.0/tycho-p2/tycho-p2-repository-plugin/assemble-repository-mojo.html#includeAllDependencies

And, if you suggest this solution, shouldn't it be a part of this PR to satisfy self-containment requirement for SimRel contributions?

I updated this PR to set includeAllDependencies to true for those repos that contain the passage-oshi feature. But I'm not sure if each of your repos is really self-contained at the moment. For example org.eclipse.passage.lic.agreements.model requires org.eclipse.emf.ecore but that is not available in the lic-repo but only in the lbc-repo. I assume you want them to be self contained as composite? In that case that tycho property should not be used because it makes each repo self-contained. Instead I suggest to move slf4j into the org.eclipse.passage.lbc.target.feature and move it to the ldc-repo instead. Because the latter feature is contibuted to SimRel so it is not an issue for the SimRel.

But for example org.eclipse.core.runtime, which is required by org.eclipse.passage.lbc.jetty or org.eclipse.jetty.http required by org.eclipse.passage.lic.jetty also seem to be missing from the Passage repos.

@HannesWell HannesWell force-pushed the removeSLF4JFromFeature branch from 803278f to e8011c6 Compare October 3, 2022 20:17
@ruspl-afed
Copy link
Contributor

Only org.eclipse.passage.ldc.repository is contributed to SimRel (LDC, not LBC).
But, we are actively using other Passage update site URLs for a number of use cases and would like to keep them functional.

@HannesWell
Copy link
Contributor Author

Only org.eclipse.passage.ldc.repository is contributed to SimRel (LDC, not LBC). But, we are actively using other Passage update site URLs for a number of use cases and would like to keep them functional.

Understand. And moving slf4j from the ldc and lic repo to the lbc repo would break the former two?
Because for the reasons mentioned in my previous comment I suspect that ldc and lic are not used/usable without lbc and moving slf4j would therefore not make a difference.
And that is what the current status of this PR is doing.

@HannesWell HannesWell force-pushed the removeSLF4JFromFeature branch from e8011c6 to 1903fd5 Compare October 5, 2022 21:04
@ruspl-afed
Copy link
Contributor

I suspect that ldc and lic are not used/usable without lbc

Nope, this is not the case

  • LIC is Licensing Integration Components, it contains Passage engine and should not depened on any other components
  • LBC is Licensing Backend Components (that requires LIC), it is used for FLS that is not mandatory.
  • LDC is PDE integration (that requires LIC) and a part of SimRel (together with LIC)

This is why moving SLF4J from LIC to LBC will make a notable difference

@HannesWell
Copy link
Contributor Author

I suspect that ldc and lic are not used/usable without lbc

Nope, this is not the case

* LIC is Licensing Integration Components, it contains Passage engine and should not depened on any other components

Maybe I'm using the wrong repositories but for example the passage/updates/release/2.5.0/lic is not self-contained for all features/plug-ins. The following target-definition:

<target name="passage-repos">
	<locations>
		<location includeAllPlatforms="false" includeConfigurePhase="true" includeMode="planner" includeSource="true" type="InstallableUnit">
			<repository location="https://download.eclipse.org/passage/updates/release/2.5.0/lic/"/>
			<unit id="org.eclipse.passage.lic.agreements.feature.feature.group" version="0.0.0"/>
		</location>
	</locations>
</target>

... does not resolve because of those errors:

grafik

In a similar way the following target-definition does not resolve:

<target name="passage-repos">
	<locations>
		<location includeAllPlatforms="false" includeConfigurePhase="true" includeMode="planner" includeSource="true" type="InstallableUnit">
			<repository location="https://download.eclipse.org/passage/updates/release/2.5.0/lic/"/>
			<unit id="org.eclipse.passage.lic.execute.feature.feature.group" version="0.0.0"/>
		</location>
	</locations>
</target>

grafik

I have not checked all IUs from that repo but at least some seem not to be installable without other repos.

However if you prefer to keep slf4j in the lic and ldc repo I can just add the slf4j plugin and its source directly to the repo definition. So if somebody needs it from one of those repositories, it is available, but not pulled into the SimRel repo because it is not in the feature anymore.

@HannesWell HannesWell force-pushed the removeSLF4JFromFeature branch from 1903fd5 to 6952e40 Compare October 8, 2022 08:59
@HannesWell
Copy link
Contributor Author

The same problematic also applies to the org.apache.logging.log4j in the org.eclipse.passage.lic.equinox.feature. If one for example want's to use a more recent version of log4j from Maven-Central, but uses this feature. That feature pulls in the 'old' version from Eclipse-Orbit.

@HannesWell HannesWell force-pushed the removeSLF4JFromFeature branch 2 times, most recently from e375ff0 to 63e07d5 Compare October 17, 2022 08:33
@HannesWell
Copy link
Contributor Author

@ruspl-afed can we please proceed here as well?

If a Feature includes a Plugin, it usually includes it with a specific
name and the version that was in the TP when the feature was build.
This prevents consumers from using a slf4j bundle with different
symbolic-name or different version in their TP or product.

This is part of
eclipse-platform/eclipse.platform.releng.aggregator#588
If a Feature includes a Plugin, it usually includes it with a specific
name and the version that was in the TP when the feature was build.
This prevents consumers from using a log4j bundle with different
symbolic-name or different version in their TP or product.
@HannesWell HannesWell force-pushed the removeSLF4JFromFeature branch from 63e07d5 to 7d208fa Compare October 27, 2022 16:35
@HannesWell
Copy link
Contributor Author

Any updates on this one?

Copy link
Contributor

@ruspl-afed ruspl-afed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I don't have strong arguments against this one and we still have some room to test and revert it if things go wrong.

@ruspl-afed ruspl-afed merged commit 6970302 into eclipse-passage:master Nov 14, 2022
@HannesWell
Copy link
Contributor Author

Thank you.

@HannesWell HannesWell deleted the removeSLF4JFromFeature branch November 14, 2022 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants