Link to java sources into native fragments to compile them in fragments #1008
Conversation
|
This change would be much simpler, if we could link the (direct) content of the selected folder directly together using a link-filter. |
HannesWell
force-pushed
the
link-native-sources
branch
from
5a00773 to
1bfbf29
Compare
|
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: |
HannesWell
force-pushed
the
link-native-sources
branch
from
1bfbf29 to
a0e7a2d
Compare
|
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.
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. |
|
But besides this API-Tooling problems, do you see general problems with this approach? |
|
Assuming API tooling would work, it could work :) 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
force-pushed
the
link-native-sources
branch
from
a0e7a2d to
e0411a1
Compare
Great. :)
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.
It is possible, at least on my Windows computer to import the fragments for all platforms and they all compile well. |
HannesWell
force-pushed
the
link-native-sources
branch
from
e0411a1 to
6d89c07
Compare
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 |
@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? But looking at But for org.eclipse.swt we could probably just disable the baseline checks if the bundle has no java classes. |
HannesWell
force-pushed
the
link-native-sources
branch
3 times, most recently
from
c4572a5 to
e303dc0
Compare
I have now adapted the preferences shared by all fragments to the one of 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. 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
force-pushed
the
link-native-sources
branch
from
e303dc0 to
e78b676
Compare
|
For me this PR is ready and as far as I can test it (on Windows) everything works well. |
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. |
|
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 So I tested the above scenario and it works. Looks good to me, many thanks! |
There was a problem hiding this comment.
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.
|
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 |
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
force-pushed
the
link-native-sources
branch
from
e78b676 to
9e1bbba
Compare
|
Thank you all for testing.
Did that in the update as well.
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.
Not yet. Enabling API checks in the CI is done in #1011. |
|
Thanks for your work on this! |
| <attributes> | ||
| <attribute value="org.eclipse.swt.win32.win32.x86_64" name="org.eclipse.jdt.launching.CLASSPATH_ATTR_LIBRARY_PATH_ENTRY"/> | ||
| </attributes> |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
@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?)
There was a problem hiding this comment.
Yes, the change would need to be done in every fragment's classpath file.
There was a problem hiding this comment.
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 :/
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.
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.
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.
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. |
|
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: 
|
Which branch / PR is it? What doesn't work? If you can rebase it, that should solve problems. |
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.