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

Restore CLASSPATH_ATTR_LIBRARY_PATH_ENTRYs in native .classpath files #1031

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

HannesWell
Copy link
Member

@HannesWell HannesWell commented Feb 5, 2024

Without that launching standalone Java SWT application from the development workspace fails to load the native binaries of the platform, as it was discovered in #1008 (comment).

Unfortunately it looks like variable resolving does not work (at least it didn't work for me locally), altough this old forum post says it should work to resolve variables in org.eclipse.jdt.launching.CLASSPATH_ATTR_LIBRARY_PATH_ENTRY's value:
https://www.eclipse.org/forums/index.php/t/152868/

Does anybody now if there is something else that needs to be done?

Copy link
Contributor

github-actions bot commented Feb 5, 2024

Test Results

   299 files  ±0     299 suites  ±0   6m 19s ⏱️ +24s
 4 100 tests ±0   4 092 ✅ ±0   8 💤 ±0  0 ❌ ±0 
12 212 runs  ±0  12 137 ✅ ±0  75 💤 ±0  0 ❌ ±0 

Results for commit 4726553. ± Comparison against base commit 2ca0b37.

♻️ This comment has been updated with latest results.

@Phillipus
Copy link
Contributor

Phillipus commented Feb 5, 2024

Can you declare multiple attributes in the .classpath files, like:

<attributes>
	<attribute value="org.eclipse.swt.cocoa.macosx.aarch64" name="org.eclipse.jdt.launching.CLASSPATH_ATTR_LIBRARY_PATH_ENTRY"/>
	<attribute value="org.eclipse.swt.cocoa.macosx.x86_64" name="org.eclipse.jdt.launching.CLASSPATH_ATTR_LIBRARY_PATH_ENTRY"/>
</attributes>

Edit - no, cancel that, sorry!

@Phillipus
Copy link
Contributor

Phillipus commented Feb 5, 2024

And now the {project_name} seems to be working on Mac. It fails on first run, then is OK on second run.

@HannesWell
Copy link
Member Author

Edit - no, cancel that, sorry!

Too bad.

And now the {project_name} seems to be working on Mac.

It works if you have created the launch config while you had set an explicit value of the attribute.
If you go open the launch config of a standalone SWT app and click Show Command Line you will see the previous value for
-Djava.library.path=. If you re-create the launch config, e.g. with one of the example snippets, then the snippet project is used as value.

I think somebody has to check JDT, why the variable replacement does not work (or if it is implemented at all). I can try to have a look later, but if somebody else is interested in doing that, help would be more than welcome. :)

@Phillipus
Copy link
Contributor

I think somebody has to check JDT, why the variable replacement does not work (or if it is implemented at all). I can try to have a look later, but if somebody else is interested in doing that, help would be more than welcome. :)

Sorry, I don't have enough JDT-Fu to do that. And it might take some time for JDT to look at it.

In the meantime, as this breaks SWT Snippet testing, what about providing explicit separate .classpath_X files?

Without that launching standalone Java SWT application from the
development workspace fails to load the native binaries of the platform.
@Phillipus
Copy link
Contributor

@HannesWell Your latest commit seems to be working on Mac for both aarch64 and x86_64. I didn't test Windows or Linux.

@HannesWell
Copy link
Member Author

I think somebody has to check JDT, why the variable replacement does not work (or if it is implemented at all). I can try to have a look later, but if somebody else is interested in doing that, help would be more than welcome. :)

Sorry, I don't have enough JDT-Fu to do that. And it might take some time for JDT to look at it.

In the meantime, as this breaks SWT Snippet testing, what about providing explicit separate .classpath_X files?

I looked into this at JDT and it is actually working. The problem is just that ${project_name}, if no argument is specified, resolves to the name of the currently selected project, which is of course the org.eclipse.swt.snippets projects if one selects to run a SWT example snippet. After thinking about it again, what we actually want is using org.eclipse.swt.${target.ws}.${target.os}.${target.arch} as value of the CLASSPATH_ATTR_LIBRARY_PATH_ENTRY attribute.
I have updated this PR accordingly. On my Windows computer this worked well.

@HannesWell HannesWell marked this pull request as ready for review February 5, 2024 22:52
@HannesWell
Copy link
Member Author

@HannesWell Your latest commit seems to be working on Mac for both aarch64 and x86_64. I didn't test Windows or Linux.

Thanks a lot for the quick tests.
With Mac and Windows being tested, I think it is save enough to assume that Linux will work too (the classpath-attribute is char-by-char equal on all three platforms). So lets submit this now.

@HannesWell HannesWell merged commit 5a9e488 into eclipse-platform:master Feb 5, 2024
10 of 13 checks passed
@HannesWell HannesWell deleted the fix-lib-path branch February 5, 2024 22:55
@Phillipus
Copy link
Contributor

So lets submit this now.

+1

@HeikoKlare Please test.

@laeubi
Copy link
Contributor

laeubi commented Feb 6, 2024

Can you declare multiple attributes in the .classpath files, like:

Shouldn't there be one classpath file per fragment anyways? Even though there might be some duplication these files actually never change, same for the settings files.

I just wanted to mention this as Tycho currently does not support linked files in all deepth so some features (especially API tools and probably pomless as well) wont work as expected if they can't find the information, see for example:

@HeikoKlare
Copy link
Contributor

I've tested the changes on Windows and Linux and both work as expected. Thanks, Hannes!

Shouldn't there be one classpath file per fragment anyways? Even though there might be some duplication these files actually never change, same for the settings files.

That sounds reasonable and I'd also prefer that.

@Phillipus
Copy link
Contributor

Shouldn't there be one classpath file per fragment anyways? Even though there might be some duplication these files actually never change, same for the settings files.

@HannesWell Is this something you are considering?

@HannesWell
Copy link
Member Author

Shouldn't there be one classpath file per fragment anyways? Even though there might be some duplication these files actually never change, same for the settings files.

@HannesWell Is this something you are considering?

I definitely considered this, but I'm actually quite happy that it was possible to just link these resources and I put effort into making it possible (like this PR).

Correct me if I'm wrong, but as far as I can tell there is no real problem with the current linking approach, even the API-check in Tycho seems to work.

And even if the .classpath files change often I think it is good to have them linked to make it clear and ensure they are identical for each os.
And the settings do change. For example each time the Java EE requirement/bundle required execution environment changes the setting change or if for example the formatter settings are adjusted.

Or do you see any real disadvantages of this approach?

@Phillipus
Copy link
Contributor

Or do you see any real disadvantages of this approach?

I personally like your approach.

I asked "Is this something you are considering?" just to see if this approach will remain as is. I think, though, that @laeubi and @HeikoKlare have some concerns?

@HannesWell
Copy link
Member Author

I'm glad you like it.

I asked "Is this something you are considering?" just to see if this approach will remain as is.

In general I would be in favor of that, but I'm currently working on making the project setup even more Maven oriented, which eventually will allow us to add an OSGi requirement from the host to the fragment, which ensures a suitable fragment is always present in an OSGi runtime. And with that a few things will change again, but I personally don't plan to change the linking in general at the moment.

@HeikoKlare
Copy link
Contributor

I also really like the current approach and highly appreciate the effort Hannes has already put into improving the SWT setup!

I don't have any concerns regarding the current approach. I just wanted to mention that Christoph's proposal in #1031 (comment) of having one .classpath file per fragment sounds reasonable to me. But that was just a comment and nothing that needs to be addressed, in particular if the setup will further change anyway.

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.

4 participants