Skip to content

Fix svg fragment build #2077

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

Merged
merged 2 commits into from
May 1, 2025
Merged

Fix svg fragment build #2077

merged 2 commits into from
May 1, 2025

Conversation

akurtakov
Copy link
Member

Ensure that the applicable implementation fragments are available for svg fragment
Part of
eclipse-platform/eclipse.platform.releng.aggregator#3006

akurtakov added 2 commits May 1, 2025 10:35
Ensure that the applicable implementation fragments are available for
svg fragment
Part of
eclipse-platform/eclipse.platform.releng.aggregator#3006
@HeikoKlare
Copy link
Contributor

In case this does not solve the problem, we may adopt what we found when taking a look at the same problem popping up when trying to get the aggregator build working with GHA: eclipse-platform/eclipse.platform.releng.aggregator#2999
I my lautest tests are correct, it would suffice to install the SWT fragments before doing the full Maven build. Seems like we did not face the issue with the wrong build order before as the native SWT fragments required to to build SVG fragments were retrieved from some repo.
It might be possible to have that quick fix applied in the I-Build as well.

@merks
Copy link
Contributor

merks commented May 1, 2025

FYI. We tested this with a local build where it worked.

Copy link
Contributor

github-actions bot commented May 1, 2025

Test Results

   539 files  ±0     539 suites  ±0   29m 45s ⏱️ -3s
 4 337 tests ±0   4 321 ✅ ±0   15 💤 ±0  1 ❌ ±0 
16 601 runs  ±0  16 463 ✅ ±0  137 💤 ±0  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit 626d012. ± Comparison against base commit dcc8cd1.

@merks
Copy link
Contributor

merks commented May 1, 2025

I see the ci build successfully built the svg jar.

@HeikoKlare
Copy link
Contributor

Alright. You just need to make sure that you locally build against a clean m2 cache as otherwise the SWT fragments will be taken from there.

@akurtakov
Copy link
Member Author

Windows test failure unrelated. Merging.

@akurtakov akurtakov merged commit 788ba23 into eclipse-platform:master May 1, 2025
15 of 17 checks passed
@github-project-automation github-project-automation bot moved this to Done in SWT work May 1, 2025
@merks
Copy link
Contributor

merks commented May 1, 2025

Alright. You just need to make sure that you locally build against a clean m2 cache as otherwise the SWT fragments will be taken from there.

Can you explain what you believe is the potential problem? Why would the fragments come from the cache instead of the reactor. Isn't it the case that the classpath seen by the fragment comes from the host, org.eclipse.swt, and it has very strict requirements on the specific current versions of the swt fragments via its p2.inf?

@HeikoKlare
Copy link
Contributor

I have to admit that I did not really understand the caused yet but just started analyzing the symptoms to get some overview. What I found is that when installing the SWT bundles/fragments to the used m2 cache before executing the aggregator build, it work, whereas without the preceding installation it fails with the error we have seen here. What we also see is that when executing the aggregator build with profile build-individual-bundles, the build order is correct and SVG fragment is successfully built after the native fragments.

When taking a look at the SWT fix, I think it might even be the right thing to do. If I am not mistaken, we basically wanted to achieve the exact same thing with using the local-build-fragment as parent but that only works with the above mentioned profile being activated, which is not the case for the aggregator build.

What just came into my mind: when having these requirements via p2.inf added, can you still just install, e.g., the native Windows fragment and the SVG fragment to start an application or will you be forced to now have all native fragments installed?

@merks
Copy link
Contributor

merks commented May 3, 2025

Keep in mind that the host has the same requirements on the fragments:

https://github.com/eclipse-platform/eclipse.platform.swt/blob/master/bundles/org.eclipse.swt/META-INF/p2.inf

They are just augmented with a slightly more complex filter:

(&(osgi.os=win32)(osgi.ws=win32)(osgi.arch=x86_64)(!(org.eclipse.swt.buildtime=true)))

The additional buildtime filter is simply there so that it can be set to true at build time such that the requirement is filtered out to avoid circular from host to fragment and fragment to hosti.e., all these requirements only apply when that profile property is not true and, as such, they are applicable only at install time. At install time, the profile properties, osgi.os, osgi.ws, and osgi.arch are specified to match the current target combination so that only a single one those requirements is active/visible. All this is done to avoid needing to install the swt bundle via a feature that includes the fragments, also with these same filtered requirements so that only one fragment is installed.

Adding these almost-the-same requirements to the svg fragment will have no different impact at install time other than to force the appropriate native fragment to be installed, which is kind of redundant because the those must also be installed because the host already has the necessary requirements. Moreover the host has very strict version range requirements, so the svg requirements without version constraints are not actually going to allow an arbitrary version of the fragment to be installed because the host ensures that an exactly-matching native one is installed.

So the only purpose of these requirements is to convince Tycho to build the native fragments before the svg fragment which is the goal here. I don't think this will impact which version of fragment (from reactor or from .m2 cache) s used to compile the code.

@HeikoKlare
Copy link
Contributor

Thank you for the great explanation!

At install time, the profile properties, osgi.os, osgi.ws, and osgi.arch are specified to match the current target combination so that only a single one those requirements is active/visible.

That's the part I mentally skipped as I thought there would be a requirement to all fragments and not only the one fitting the current platform. And the buildtime filter in the host requirements is just used to ensure that during build the host has a requirement to every fragment (and not only the one fitting to the current platform).

So everything is obviously fine as is.

We originally added the local-build-parent to the SVG fragment as that provides the exact same p2.inf already, but unfortunatly that is only added when build-individual-bundles is true, so it does not apply during aggregator builds.

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.

3 participants