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

Minor simplifications and clean-ups in Toolchain processing #2380

Merged
merged 1 commit into from
Sep 24, 2023

Conversation

HannesWell
Copy link
Member

Replace deprecated DefaultJavaToolChain by JavaToolchainImpl.

Initially part of #2379, but there seem to be some bugs in this change.
@laeubi do you directly have any hot hint?

@github-actions
Copy link

github-actions bot commented May 3, 2023

Test Results

   561 files  ±0     561 suites  ±0   4h 27m 3s ⏱️ + 14m 4s
   363 tests ±0     357 ✔️ ±0    6 💤 ±0  0 ±0 
1 089 runs  ±0  1 070 ✔️ ±0  19 💤 ±0  0 ±0 

Results for commit fbdd082. ± Comparison against base commit 7a94e1c.

♻️ This comment has been updated with latest results.

@laeubi
Copy link
Member

laeubi commented May 18, 2023

@HannesWell I have no immediate idea, especially as these are maven classes, you cna try to even split this more into some simplifications that are maybe less critical, beside that, in the past it has worked well to abstract the maven parts (e.g by doing as much as possible in tycho-core) instead of any of the mojos,, I even using reflection on some places :-\

@laeubi laeubi force-pushed the cleanUpToolchainProcessing branch from f21cf8d to b552c51 Compare June 15, 2023 17:56
@laeubi
Copy link
Member

laeubi commented Sep 23, 2023

@HannesWell do you plan to work on this or should we postpone this until Maven 4?

@HannesWell
Copy link
Member Author

I temporarily reverted the replacement of the deprecated DefaultJavaToolChain. Lets see if this makes a difference. I also checked again the other changes and actually they look save, but I might have overseen something.

@HannesWell
Copy link
Member Author

I temporarily reverted the replacement of the deprecated DefaultJavaToolChain. Lets see if this makes a difference. I also checked again the other changes and actually they look save, but I might have overseen something.

OK, that worked. But I don't understand why it breaks something if we just use the super-class.
Lets try it again and the previous failure was just a temporary issue.
If it still fails, lets submit this with the deprecated DefaultJavaToolChain in place.

Replace deprecated DefaultJavaToolChain by JavaToolchainImpl.
@HannesWell HannesWell marked this pull request as ready for review September 24, 2023 08:57
@HannesWell
Copy link
Member Author

HannesWell commented Sep 24, 2023

Looks like this worked now.
Could it be that the test-runners where using a too old Maven version?
Any way, @laeubi any objection to merge this now?

@laeubi
Copy link
Member

laeubi commented Sep 24, 2023

Could it be that the test-runners where using a too old Maven version?

I have no clue, as it works now I don't see a reason to further wait with this.

@laeubi laeubi merged commit 3118388 into eclipse-tycho:master Sep 24, 2023
8 checks passed
@HannesWell HannesWell deleted the cleanUpToolchainProcessing branch September 24, 2023 13:26
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.

2 participants