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

Set the real output location for embedded jars in ApiAnalysis #2996

Merged
merged 2 commits into from
Nov 7, 2023

Conversation

laeubi
Copy link
Member

@laeubi laeubi commented Nov 6, 2023

Currently when one configures embedded extra jars these are not found by the ApiAnalysis because they are placed in different location than the main classes.

This computes all jars and compares there resulting path with the current output location updating those if needed.

See

Copy link

github-actions bot commented Nov 6, 2023

Test Results

   570 files  ±0     570 suites  ±0   4h 27m 39s ⏱️ + 13m 0s
   371 tests +1     364 ✔️ ±0    6 💤 ±0  0 ±0  1 🔥 +1 
1 113 runs  +3  1 093 ✔️ +2  19 💤 ±0  0 ±0  1 🔥 +1 

For more details on these errors, see this check.

Results for commit fa46d2d. ± Comparison against base commit 5cddc68.

This pull request removes 1 and adds 2 tests. Note that renamed tests count towards both.
org.eclipse.tycho.test.apitools.ApiToolsTest ‑ testVerify
org.eclipse.tycho.test.apitools.ApiToolsTest ‑ testApiBreak
org.eclipse.tycho.test.apitools.ApiToolsTest ‑ testEmbeddedJars

♻️ This comment has been updated with latest results.

@laeubi
Copy link
Member Author

laeubi commented Nov 7, 2023

I was not yet able to reproduce the problem with an integration test, but with the change the PDE PR builds fine...

@laeubi laeubi added the backport-to-tycho-4.0.x Can be added to a PR to trigger an automatic backport of the change label Nov 7, 2023
Currently when one configures embedded extra jars these are not found by
the ApiAnalysis because they are placed in different location than the
main classes.

THis computes all jars and compares there resulting path with the
current output location updating those if needed.
@laeubi laeubi force-pushed the support_custom_lib_path branch 3 times, most recently from dc9a3fb to d4d4c24 Compare November 7, 2023 08:48
@laeubi laeubi marked this pull request as ready for review November 7, 2023 08:48
@laeubi
Copy link
Member Author

laeubi commented Nov 7, 2023

I now used a stripped down version of the pdebuild bundle as a testcase what failed before my change but succeeds with my change.

@laeubi laeubi merged commit c7733ee into eclipse-tycho:master Nov 7, 2023
8 of 10 checks passed
@jukzi
Copy link

jukzi commented Nov 8, 2023

@laeubi @iloveeclipse Is it possible that this commit introduced missing output.. = bin/ in build.properties to not use org.eclipse.jdt.internal.core.JavaProject.defaultOutputLocation() leading to almost every build to fail now with API errors?

@laeubi
Copy link
Member Author

laeubi commented Nov 8, 2023

leading to almost every build to fail now with API errors

Only those fail with missing information in build.properties... what is the primary input and source of truth for Tycho.

@jukzi
Copy link

jukzi commented Nov 8, 2023

We have thousends of build.properties in our workspace... . It's well defined that org.eclipse.jdt.core.IJavaProject.getOutputLocation() uses a default. Can you please roll back the change that caused this regression or fix it?

@laeubi
Copy link
Member Author

laeubi commented Nov 8, 2023

We have thousends of build.properties in our workspace... . It's well defined that org.eclipse.jdt.core.IJavaProject.getOutputLocation() uses a default.

IJavaProject.getOutputLocation() does not use a default it is defined in .classpath but has nothing in relation to build.properties

Can you please roll back the change that caused this regression

No, beside that without a failing test there is no regression.

or fix it?

Maybe but not before the next release so one should simply apply the easy to apply provided mitigation.

@jukzi
Copy link

jukzi commented Nov 8, 2023

Its not feasible to update thousands of files. You are effectively blocking all development with that. That's totally inappropriate.

@laeubi
Copy link
Member Author

laeubi commented Nov 8, 2023

Its not feasible to update thousands of files. You are effectively blocking all development with that. That's totally inappropriate.

We are in freeze time until end of release, it is totally inappropriate to modify "thousands" of bundles...
Only bugfixes should happen on a small amount of code, freeze is not meant to perform mass changes.

Also it is searching for "output.. = bin/" shows 399 matches in the SDK where the full build 520 modules (not all are bundles though) so at a maximum there are 121 what is not "thousands" not even one thousand.

Beside that everyone is free to provide a Testcase + PR to fix that and trigger the necessary steps for a new release afterwards if one thinks it is really critical or use any of the older releases of Tycho.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-tycho-4.0.x Can be added to a PR to trigger an automatic backport of the change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants