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

Make m2e.core.ui host of archetype-fragments and delete archetype.common #1494

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

HannesWell
Copy link
Contributor

The Plugin o.e.m2e.archetype.common is empty and only serves as host for the maven-archetype fragment-bundles. Since they are only used by m2e.core.ui, we can simply make that the host of those fragments and save one project in m2e.

@laeubi what do you think?

@github-actions
Copy link

github-actions bot commented Jul 30, 2023

Test Results

106 files  ±0  106 suites  ±0   11m 7s ⏱️ - 2m 43s
660 tests ±0  650 ✔️ +1  10 💤 ±0  0  - 1 
660 runs  ±0  649 ✔️ +1  11 💤 ±0  0  - 1 

Results for commit f1380c6. ± Comparison against base commit 837d904.

♻️ This comment has been updated with latest results.

@laeubi
Copy link
Member

laeubi commented Jul 31, 2023

I don't think this is really worth because it require special treatment of the m2.ui bundle.
If you think we really should save bundles, one can make the archtype common the host bundle (by adding an extra bnd mapping for it to the TP) and the other ones fragments of that bundle but be aware that this is actually a major change for the feature.

@HannesWell HannesWell force-pushed the archetypeHost branch 3 times, most recently from 34b9695 to 2055eca Compare August 19, 2023 20:43
@HannesWell
Copy link
Contributor Author

one can make the archtype common the host bundle (by adding an extra bnd mapping for it to the TP) and the other ones fragments of that bundle

Did that and also re-used the jdom2 bundle provided by maven-bnd Orbit, so that we don't have to define an own one.

but be aware that this is actually a major change for the feature.

I wouldn't consider this a braeking change in thise case since the bundles contained in this PDE are more internal and the same feature (in a functional sense is still provided). But I worked around this, simply by keep the generated bundle name as org.eclipse.m2e.archetype.common.

Comment on lines +29 to +55
version="0.0.0"
fragment="true"
unpack="false"/>

<plugin
id="org.eclipse.m2e.archetype.catalog"
download-size="0"
install-size="0"
version="0.0.0"
fragment="true"
unpack="false"/>

<plugin
id="org.eclipse.m2e.archetype.descriptor"
download-size="0"
install-size="0"
version="0.0.0"
fragment="true"
unpack="false"/>

<plugin
id="org.eclipse.m2e.archetype.maven-artifact-transfer"
download-size="0"
install-size="0"
version="0.0.0"
fragment="true"
unpack="false"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@laeubi do you see any reason that speaks against including the archetype bundles in this feature?
I would prefer it over the p2.inf file since it is less magic.

Copy link
Member

@laeubi laeubi Aug 20, 2023

Choose a reason for hiding this comment

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

The p2.inf ensures that all fragments are always installed together (especially for cases like tests).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The p2.inf ensures that all fragments are always installed together (especially for cases like tests).

This can also be achieved by corresponding Import-Packages and is less magic and even less to type.

Copy link
Member

Choose a reason for hiding this comment

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

No this can't work because a fragment is always optional, so a consumer would need to import a package from each fragment even if it is never used.

Copy link
Contributor Author

@HannesWell HannesWell Aug 20, 2023

Choose a reason for hiding this comment

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

No this can't work because a fragment is always optional, so a consumer would need to import a package from each fragment even if it is never used.

That's right, a fragment is optional but m2e.core.ui already needs to import a package from each of the archetype artifacts (if one does not use require-bundle). The only import that would not be necessary from a class-loading/compiling point of view is the maven-transfer. But I think one not so necessary import is IMHO ok. And just to save that I wouldn't add/keep the p2.inf.

@HannesWell
Copy link
Contributor Author

@laeubi this change is IMHO now ready, do you have any objects?

Bundle-Name: M2Eclipse supplement fragment for ${mvnGroupId}:${mvnArtifactId}:${mvnVersion}
version: ${version_cleanup;${mvnVersion}}
Bundle-SymbolicName: org.eclipse.m2e.archetype.common
Bundle-Version: ${version}.1
Copy link
Member

Choose a reason for hiding this comment

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

this will result in a 3.2.1.1 version is this really desired?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is.
It is a (manual) qualifier bump so that those that use the snapshots don't miss the update.
When there is a new archetype version, it can be removed.

Copy link
Member

Choose a reason for hiding this comment

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

1 is really a bad qualifier, why not do like in maven-runtime and use

${version}01 what will result in 3.2.101

@HannesWell HannesWell force-pushed the archetypeHost branch 3 times, most recently from d870fbf to 0d82317 Compare August 22, 2023 08:01
The Plugin o.e.m2e.archetype.common is empty and only serves as host for
the maven-archetype fragment-bundles. Since they are only used by
m2e.core.ui, we can simply make that the host of those fragments and
save one project in m2e.

In o.e.m2e.core.ui import all required packages plus those that are from
fragments that are also needed for archetype.common to function
properly, even if the package is not directly needed in m2e.core.ui.
@HannesWell HannesWell merged commit d2ec2f0 into eclipse-m2e:master Aug 22, 2023
4 of 5 checks passed
@HannesWell HannesWell deleted the archetypeHost branch August 22, 2023 22:17
@HannesWell HannesWell added this to the 2.4.0 milestone Aug 27, 2023
HannesWell added a commit to HannesWell/m2e-core that referenced this pull request Aug 31, 2023
The Plug-in org.eclipse.m2e.archetype.common is not a Fragment and the
corresponding attribute was added erroneously.
This was accidentally added in
eclipse-m2e#1494

Reported in eclipse-m2e#1537
HannesWell added a commit to HannesWell/m2e-core that referenced this pull request Aug 31, 2023
The Plug-in org.eclipse.m2e.archetype.common is not a Fragment and the
corresponding attribute was added erroneously.
This was accidentally added in
eclipse-m2e#1494

Reported in eclipse-m2e#1537
HannesWell added a commit that referenced this pull request Aug 31, 2023
The Plug-in org.eclipse.m2e.archetype.common is not a Fragment and the
corresponding attribute was added erroneously.
This was accidentally added in
#1494

Reported in #1537
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