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

Link to java sources into native fragments to compile them in fragments #1008

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

HannesWell
Copy link
Member

Previously within the IDE workspace, the SWT java-sources for a platform have been compiled within the org.eclipse.swt project using a .classpath file specially crafted for each supported window-system/platform. Therefore when checking out the SWT sources in the IDE one had to rename the '.classpath_${ws}' template for the desired platform to '.classpath' to use it locally.
With this change this renaming is not necessary anymore.

Furthermore it allows PDE API-Tools to remove the built-in special handling of the org.eclipse.swt setup because now the API baseline check within the IDE can happen simply in the fragment as it exists in the baseline.

@HannesWell
Copy link
Member Author

This change would be much simpler, if we could link the (direct) content of the selected folder directly together using a link-filter.
Currently it seems that only the link target is made available.

Copy link
Contributor

github-actions bot commented Jan 28, 2024

Test Results

   299 files  ±0     299 suites  ±0   5m 44s ⏱️ -20s
 4 098 tests ±0   4 090 ✅ ±0   8 💤 ±0  0 ❌ ±0 
12 206 runs  ±0  12 133 ✅ ±0  73 💤 ±0  0 ❌ ±0 

Results for commit 9e1bbba. ± Comparison against base commit 6e665a6.

♻️ This comment has been updated with latest results.

@iloveeclipse
Copy link
Member

Not sure about that, right now because of API tooling. It shows for the new fragment project ~ 2400 new warnings of type "X illegally references field Y". May be because of API filters missing that were set on original project.

Also now I see API error on SWT:
The minor version should be the same for version 3.125.0, since no new APIs have been added since version 3.124.200 MANIFEST.MF /org.eclipse.swt/META-INF Version Numbering Problem

@HannesWell
Copy link
Member Author

On my Windows Computer all fragments are error free if I import them in the IDE and it looks like they all compile the sources.

Have you tried it with deleting and re-creating the baseline? This usually helps for me if it went crazy.

Also now I see API error on SWT:
The minor version should be the same for version 3.125.0, since no new APIs have been added since version 3.124.200 MANIFEST.MF /org.eclipse.swt/META-INF Version Numbering Problem

I got that one as well, and added a filter for it.

But I don't understand the build-failure. I only see warnings, no errors. Have to investigate that later.

@HannesWell
Copy link
Member Author

But besides this API-Tooling problems, do you see general problems with this approach?

@iloveeclipse
Copy link
Member

Assuming API tooling would work, it could work :)
I also haven't checked how debugging would work while debugging IDE from IDE.

Haven't tried, but it looks like one could also open and compile & run API tools on all fragment projects in one workspace - this was not possible before. Would be useful for PR's that add API in fragments.

@HannesWell
Copy link
Member Author

Assuming API tooling would work, it could work :)

Great. :)
The build now succeeds. It failed because the org.eclipse.swt plugin exported the package org.eclipse.swt.internal with swt.tools.spies as friend:
org.eclipse.swt.internal;x-friends:="org.eclipse.ui,org.eclipse.swt.tools.spies"
But all the fragments don't do that. I now just added a suppression of the restriction warnings for the Sleak class.

I also haven't checked how debugging would work while debugging IDE from IDE.

I just tried it and at least debugging an IDE launched from my workspace seems to work well as usually, just as if the java sources were really located in the fragments.

Haven't tried, but it looks like one could also open and compile & run API tools on all fragment projects in one workspace - this was not possible before. Would be useful for PR's that add API in fragments.

It is possible, at least on my Windows computer to import the fragments for all platforms and they all compile well.
I also added an API error locally (removed a method) and got the expected API error, but only for the windows fragment. I haven't investigated it yet why this is the case. Maybe it is because the Eclipse-PlatformFilter lets the resolution of the fragment fail or maybe it is because the resource effectively exists multiple times in the workspace.

@HannesWell
Copy link
Member Author

Maybe it is because the Eclipse-PlatformFilter lets the resolution of the fragment fail or maybe it is because the resource effectively exists multiple times in the workspace.

One thing that was of course missing (at least with the Oomph setup) is that the fragments of other platforms are not added to the baseline. I now have adjusted the setup to includeAllPlatforms. With that only (at least after I had listed all fragments explicitly before, but I hope it just works without that), the platform-specific fragments were in the baseline.
But an error was still only at the windows fragment for me.

@HannesWell
Copy link
Member Author

Not sure about that, right now because of API tooling. It shows for the new fragment project ~ 2400 new warnings of type "X illegally references field Y". May be because of API filters missing that were set on original project.

@iloveeclipse I see about 2400 API warnings with such message in my workspace. Could it be that you have set up the API-tools to be more strict?
I have provisioned my workspace using Oomph, but I see no special preference for that so it probably is the default.

But looking at org.eclipse.swt\.settings\org.eclipse.pde.api.tools.prefs I see a lot of preferences set to ERROR, so we probably want to replicate that for the fragments with this.

But for org.eclipse.swt we could probably just disable the baseline checks if the bundle has no java classes.

@HannesWell HannesWell force-pushed the link-native-sources branch 3 times, most recently from c4572a5 to e303dc0 Compare January 29, 2024 22:15
@HannesWell
Copy link
Member Author

But looking at org.eclipse.swt\.settings\org.eclipse.pde.api.tools.prefs I see a lot of preferences set to ERROR, so we probably want to replicate that for the fragments with this.

I have now adapted the preferences shared by all fragments to the one of o.e.swt and now get those warnings as errors as expected.
For o.e.swt I have now disabled API-tools. Since the bundle is empty I think thats the better solution than adding hundes/thounds of lines of API filters.

And mentioning filters, only for the o.e.swt.win32.win32.x86_64 fragment I had to add about 3000 lines of API filters in order to silence all errors and I wonder if it is really sensible to make API problems errors just to suppress them with filters.
Why not just ignore the problem in the first place?
Especially for problems like Referencing a type, field or method tagged '@noreference' that can never be an actual problem since SWT does not have any dependencies so it can only be a reference to an internal class.
With that the vast majority of errors is gone.
In this case all the Restrictions problems don't make sense as errors:
grafik

And in general I think PDE-APITools should not have issues with internal usage/references/overrides/... of internal classes. But maybe this is a side-effect of the special handling of SWT (eclipse-pde/eclipse.pde#1084).

@HannesWell
Copy link
Member Author

HannesWell commented Jan 30, 2024

For me this PR is ready and as far as I can test it (on Windows) everything works well.
Everybody interested, please review and try out this change. Especially those on other platforms.
If no objections are raised I plan to submit this tonight.

@iloveeclipse
Copy link
Member

For o.e.swt I have now disabled API-tools.

But we still have API checks enabled & errors reported for fragments in IDE & command line build?

Could you also please update PR description to explain what the change does (it currently explains only how bad it was and how good it will be, but doesn't explain the actual change). This would simplify understanding of the changes.

@SyntevoAlex : would be nice if you could review this one too, because you seem to work with SWT on a completely different setup.
@Phillipus : could you please check this PR too?

@Phillipus
Copy link
Contributor

Well, this makes life easier. :-)

My use case when testing latest SWT on Mac is to test against Mac aarch64 (Apple Silicon) and x86_64 (Intel) targets. I can do this by switching the Architecture from aarch64 to x86_64 on the Environment tab of my .target file, and changing the target JDK to match as well. I have both the org.eclipse.swt.cocoa.macosx.aarch64 and the org.eclipse.swt.cocoa.macosx.x86_64 binary projects open in my workspace.

So I tested the above scenario and it works. Looks good to me, many thanks!

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

This is a great improvement and simplification, thank you! To me, the new structure makes more sense, as it is easier to set up and as it reflects the final artifact structure already during development time.

I went through the changes and tried them in both the IDE and a Tycho build (on Windows), and I did not find any issues. API errors are still reported in the IDE for the fragments (at least for the Windows fragment that I have checked). One thing I thought about is whether there could be some better place for the OS-specific classpath files than in the binaries parent folder, since this folder is usually not present in your workspace, which makes interaction with these files a little more "complicated". But since I do not consider this a relevant issue, I did not think further about where to better place them and it is fine more me as is.

So in short: I appreciate these changes; go from my side.

Copy link
Contributor

@Phillipus Phillipus left a comment

Choose a reason for hiding this comment

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

Adding this +1 comment here so that my name has a tick by it in the list of reviewers. :-)

@iloveeclipse
Copy link
Member

I haven't reviewed the changes so no "formal approval" from me, but I've played a bit with the PR in my workspace.

With that it looks OK from my side.

PS
Does this PR help to report API issues on SWT in Jenkins in any way, or do we need another ticket for that?

For each native SWT fragment link the source-folders containing the java
classes used in that native fragment into said fragment's Eclipse
project. This allows to compile the java classes within the fragment
project already in the development workspace and consequently to develop
and test the code in the workspace using the same structure as the built
artifacts. If desired, the code of multiple/all fragments can be
checked-out and compiled at the same time, just by opening all desired
fragment projects.

The sources still reside within the org.eclipse.swt main/host bundle but
are not compiled there anymore in the dev-workspace. This allows to
share common code while defining it only once at a central location.
Also move the shared preferences into the 'binaries/.setting' folder and
link it as '.settings' folder in each native fragment. This allows to
share the same settings in all fragment projects, while defining them
only once, just like the code.

Previously within the IDE workspace, the SWT java-sources for a platform
have been compiled within the org.eclipse.swt project using a .classpath
file specially crafted for each supported window-system/platform.
Therefore when checking out the SWT sources in the IDE one had to rename
the '.classpath_${ws}' template for the desired platform to '.classpath'
to use it locally.
With this change this renaming is not necessary anymore.

Furthermore ignore all 'Restrictions' type API problem, since SWT does
only reference its own internal classes. Because SWT does not have
external dependencies internals of other bundles cannot be referenced.
Ignoring these kind of problems avoids the need to add filters to
suppress them.

Furthermore this change allows PDE API-Tools to remove the built-in
special handling of the org.eclipse.swt setup because now the API
baseline check within the IDE can happen simply in the fragment as it
exists in the baseline.
@HannesWell
Copy link
Member Author

Thank you all for testing.
I just pushed an update to remove the CLASSPATH_ATTR_LIBRARY_PATH_ENTRY classpath entry also for GTK/Linux. For Windows/Mac it was already done before (oversaw it for Linux). Additionally I also updated the Readme-files and removed the section about the .classpath renaming.

Could you also please update PR description to explain what the change does (it currently explains only how bad it was and how good it will be, but doesn't explain the actual change). This would simplify understanding of the changes.

Did that in the update as well.

One thing I thought about is whether there could be some better place for the OS-specific classpath files than in the binaries parent folder, since this folder is usually not present in your workspace, which makes interaction with these files a little more "complicated". But since I do not consider this a relevant issue, I did not think further about where to better place them and it is fine more me as is.

It is correct that the folder is often not present, but you can still modified the files through the linked folders. The changes will then be made in the original file and all other native fragments will see them to. This way consistent preferences are ensured.
But in general I think the preferences don't change often.
With eclipse-platform/eclipse.platform#89 this could be done in a more standard way.

PS
Does this PR help to report API issues on SWT in Jenkins in any way, or do we need another ticket for that?

Not yet. Enabling API checks in the CI is done in #1011.
But to answer your other question (just for completes since the others said it too), in the IDE it works at least for the fragment of the current platform. Maybe we find out later how to enable it for all platforms.

@HannesWell HannesWell merged commit 2300b0f into eclipse-platform:master Jan 30, 2024
13 checks passed
@HannesWell HannesWell deleted the link-native-sources branch January 30, 2024 18:51
@Phillipus
Copy link
Contributor

Thanks for your work on this!

Comment on lines -8 to -10
<attributes>
<attribute value="org.eclipse.swt.win32.win32.x86_64" name="org.eclipse.jdt.launching.CLASSPATH_ATTR_LIBRARY_PATH_ENTRY"/>
</attributes>
Copy link
Contributor

Choose a reason for hiding this comment

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

@HannesWell I just found that because of removing this attribute it is not possible to start a standalone SWT snippet from the IDE anymore. With this attribute, the java.library.path refers to the fragment containing the SWT dlls, which is missing without it. Was there a specific reason why you removed the attribute? Or can we reinsert it or provide some other way to run SWT standalone snippets easily?

Copy link
Contributor

@Phillipus Phillipus Feb 5, 2024

Choose a reason for hiding this comment

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

Same on Mac .classpath.

I added the following back to Mac and it worked:

<classpathentry kind="src" path="Eclipse SWT PI/cocoa">
    <attributes>
        <attribute value="org.eclipse.swt.cocoa.macosx.aarch64" name="org.eclipse.jdt.launching.CLASSPATH_ATTR_LIBRARY_PATH_ENTRY"/>
    </attributes>
</classpathentry>

Copy link
Contributor

@Phillipus Phillipus Feb 5, 2024

Choose a reason for hiding this comment

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

@HannesWell @HeikoKlare If the attribute is re-instated then on Mac there needs to be two .classpath_cocoa files - one for aarch64 and one for x86_64. (And for Linux as well?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the change would need to be done in every fragment's classpath file.

Copy link
Member Author

@HannesWell HannesWell Feb 5, 2024

Choose a reason for hiding this comment

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

Indeed. When implementing this change I only tested it with an Eclipse application and for an OSGi runtime this was not necessary, so I assumed it was a left-over from old times.
But testing it now with a standalone SWT snippet I also see it fails, sorry for the disturbance.

But at the same time this was a welcome simplification since it allowed to re-use the same .classpath file for all macos platforms. At the .classpath file for Linux/GTK only referenced the x86_64 arch project so it will probably not have worked for development on all platforms.
To make it work we would probably have provide individual .classpath files for each fragment.
I have opened #1031 to investigate if we can use a variable for that, but it currently seems not to work :/

tjwatson added a commit to tjwatson/eclipse.platform.swt that referenced this pull request Feb 5, 2024
Some projects, like BND tools, will avoid using require-bundle
at great lengths. This becomes problematic when the APIs they
want to use do not version there `Export-Package` properly.

Many Eclipse projects, including SWT, do not version their API
packages.  They only version their individual bundles with respect
to semantic versioning.

For BND they resorted to using `Import-Package` with the
`bundle-version` and `bundle-symbolic-name` of the host. This
"worked" because the SWT API packages were defined in the
`Export-Package` header of the host bundle. But now with
PR eclipse-platform#1008 this changed and the host bundle no longer defines
the exported packages. The end result is BND tools is now
broken and has many unresolvable bundles.

Until this is sorted out, this PR simply adds back the
`Export-Package` header of the SWT host. The longer term
solution should be to properly version SWT API packages
so that consumers of the API can properly use `Import-Package`
to depend on the API.
tjwatson added a commit to tjwatson/eclipse.platform.swt that referenced this pull request Feb 5, 2024
Some projects, like BND tools, will avoid using require-bundle
at great lengths. This becomes problematic when the APIs they
want to use do not version there `Export-Package` properly.

Many Eclipse projects, including SWT, do not version their API
packages.  They only version their individual bundles with respect
to semantic versioning.

For BND they resorted to using `Import-Package` with the
`bundle-version` and `bundle-symbolic-name` of the host. This
"worked" because the SWT API packages were defined in the
`Export-Package` header of the host bundle. But now with
PR eclipse-platform#1008 this changed and the host bundle no longer defines
the exported packages. The end result is BND tools is now
broken and has many unresolvable bundles.

Until this is sorted out, this PR simply adds back the
`Export-Package` header of the SWT host. The longer term
solution should be to properly version SWT API packages
so that consumers of the API can properly use `Import-Package`
to depend on the API.
tjwatson added a commit that referenced this pull request Feb 5, 2024
Some projects, like BND tools, will avoid using require-bundle
at great lengths. This becomes problematic when the APIs they
want to use do not version there `Export-Package` properly.

Many Eclipse projects, including SWT, do not version their API
packages.  They only version their individual bundles with respect
to semantic versioning.

For BND they resorted to using `Import-Package` with the
`bundle-version` and `bundle-symbolic-name` of the host. This
"worked" because the SWT API packages were defined in the
`Export-Package` header of the host bundle. But now with
PR #1008 this changed and the host bundle no longer defines
the exported packages. The end result is BND tools is now
broken and has many unresolvable bundles.

Until this is sorted out, this PR simply adds back the
`Export-Package` header of the SWT host. The longer term
solution should be to properly version SWT API packages
so that consumers of the API can properly use `Import-Package`
to depend on the API.
@SyntevoAlex
Copy link
Member

@SyntevoAlex : would be nice if you could review this one too, because you seem to work with SWT on a completely different setup.

Indeed my setup is different, to an extent where I don't use any of the modified files. I use hand-crafted IDEA project to work with SWT.

@elsazac
Copy link
Member

elsazac commented Feb 8, 2024

I was working on an old branch of mine. It looks like I can't work on the branch now due to the change. Master branch works fine but I can see a lot of errors in the workspace right now with the old branch. I attempted rebasing, but it doesn't work. Does anyone else have this problem? Any advice on how to make the branch functional?

These are the errors I am getting in the workspace with old branch:

image

@Phillipus
Copy link
Contributor

I attempted rebasing, but it doesn't work.

Which branch / PR is it? What doesn't work?

If you can rebase it, that should solve problems.

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.

6 participants