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.

Sign up for GitHub

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

Jump to bottom

Remove now unused API-tools system-packages profiles #285

Merged
merged 1 commit into from
Aug 16, 2022
Merged

Remove now unused API-tools system-packages profiles #285

merged 1 commit into from
Aug 16, 2022

Conversation

HannesWell
Copy link
Member

With #279 and #284 the system-packages profiles embeded in the pde.api.tools plugin are not used anywhere. At least I was not able to find an occurrence.

Therefore I think they can be removed.

@HannesWell HannesWell requested a review from vik-chand August 11, 2022 23:05
@HannesWell HannesWell force-pushed the removeSystemPackagesProfiles branch from 4804e6f to ce5c86a Compare August 11, 2022 23:07
@HannesWell HannesWell marked this pull request as draft August 11, 2022 23:07
vik-chand
vik-chand reviewed Aug 12, 2022
View reviewed changes
Copy link
Member

@vik-chand vik-chand left a comment

Choose a reason for hiding this comment

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

There is a hack in org.eclipse.pde.internal.build.site.PDEState. I will recommend keeping the system packages for 4.25.

@HannesWell HannesWell force-pushed the removeSystemPackagesProfiles branch from ce5c86a to ea4be78 Compare August 13, 2022 06:54
@HannesWell
Copy link
Member Author

There is a hack in org.eclipse.pde.internal.build.site.PDEState. I will recommend keeping the system packages for 4.25.

To what hack are you referring exactly? The one just removed in #284?
Nevertheless I have no problem with holding this back until the next release-cycle.

@HannesWell HannesWell force-pushed the removeSystemPackagesProfiles branch from ea4be78 to af7ea52 Compare August 14, 2022 23:34
@github-actions
Copy link

github-actions bot commented Aug 15, 2022 •
edited
Loading

Unit Test Results

     80 files       80 suites   15m 42s ⏱️
3 274 tests 2 639 ✔️ 24 💤 611 ❌
3 373 runs  2 738 ✔️ 24 💤 611 ❌

For more details on these failures, see this check.

Results for commit d8f75c7.

♻️ This comment has been updated with latest results.

@vik-chand
Copy link
Member

Now, with ea34245
we can probably remove system packages just after M3 ( Saturday/Sunday). I tested export after removing system packages and it worked fine.

@iloveeclipse
Copy link
Member

we can probably remove system packages just after M3

Why after M3? Better we get it into M3 if the change is safe, and remove from RC1 if it turned out it is not.
Otherwise we will run into troubles removing the code short before release.

@HannesWell
Remove now unused API-tools system-packages profiles
d8f75c7 
With eclipse-pde#279 and
eclipse-pde#284 the users of those
profiles were changed to use better ways to find the available
system-packages of a Java version.
@HannesWell HannesWell force-pushed the removeSystemPackagesProfiles branch from af7ea52 to d8f75c7 Compare August 16, 2022 07:17
@HannesWell
Copy link
Member Author

we can probably remove system packages just after M3

Why after M3? Better we get it into M3 if the change is safe, and remove from RC1 if it turned out it is not. Otherwise we will run into troubles removing the code short before release.

I agree with Andrey. If we want to have this in 2022-09 it is more reasonable to submit this sooner than later.

@HannesWell HannesWell marked this pull request as ready for review August 16, 2022 07:18
@HannesWell
Copy link
Member Author

I just adjusted the code-comments to this change.

@vik-chand
Copy link
Member

Ok, in that case, we should merge ASAP.

@vik-chand
Copy link
Member

My only concern was not merging on a Tuesday ( testing day) but since I've already tested the scenario ( by using the launch after taking this change), I think it should be fine to merge today. I will test again on the next I build post the merge.

vik-chand
vik-chand approved these changes Aug 16, 2022
View reviewed changes
Copy link
Member

@vik-chand vik-chand left a comment

Choose a reason for hiding this comment

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

Tested, it looks well.

@iloveeclipse
Copy link
Member

iloveeclipse commented Aug 16, 2022 •
edited
Loading

611 test fails here too... #286

#232 needs solution.

@HannesWell HannesWell merged commit 065da8c into eclipse-pde:master Aug 16, 2022
@HannesWell
Copy link
Member Author

Ok, in that case, we should merge ASAP.

Done. :)

#232 needs solution.

Yes definitely. I'll try to look into it this evening. Maybe I'm lucky.

@HannesWell HannesWell deleted the removeSystemPackagesProfiles branch August 16, 2022 09:26
@vik-chand
Copy link
Member

Ok, in that case, we should merge ASAP.

Done. :)

#232 needs solution.

Yes definitely. I'll try to look into it this evening. Maybe I'm lucky.

The next I build is 20 min away

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vik-chand vik-chand vik-chand approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HannesWell @vik-chand @iloveeclipse