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

delete WorkbenchCommandSupport "API is scheduled for deletion" #1563

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Jan 25, 2024

fix "overriding a synchronized method without being synchronized" and several deprecation warnings

@jukzi
Copy link
Contributor Author

jukzi commented Jan 25, 2024

@vogella is it OK straight delete an unused API marked for deletion at this stage?

Copy link
Contributor

github-actions bot commented Jan 25, 2024

Test Results

   918 files  ± 0     918 suites  ±0   1h 12m 51s ⏱️ + 21m 26s
 7 433 tests  -  6   7 283 ✅  -  4  150 💤  - 2  0 ❌ ±0 
23 448 runs   - 18  22 946 ✅  - 12  502 💤  - 6  0 ❌ ±0 

Results for commit dfccd1d. ± Comparison against base commit 04a9884.

This pull request removes 6 tests.
org.eclipse.ui.tests.commands.Bug66182Test ‑ Unknown test
org.eclipse.ui.tests.commands.Bug70503Test ‑ testHandlerChangeCausesUpdate
org.eclipse.ui.tests.commands.Bug74982Test ‑ testSelectAllHandlerSendsSelectionEvent
org.eclipse.ui.tests.commands.Bug74990Test ‑ testPartIdSubmission
org.eclipse.ui.tests.commands.Bug87856Test ‑ testHandlerLeak
org.eclipse.ui.tests.keys.Bug36420Test ‑ Unknown test

♻️ This comment has been updated with latest results.

@vogella
Copy link
Contributor

vogella commented Jan 25, 2024

@vogella is it OK straight delete an unused API marked for deletion at this stage?

The process is described here: https://github.com/eclipse-platform/eclipse.platform/blob/master/docs/ApiRemovalProcess.md I don't see the annotation in your commit so you need to check (and potentially update) https://help.eclipse.org/latest/index.jsp?topic=%2Forg.eclipse.platform.doc.isv%2Fporting%2Fremovals.html if the freeze period for this API is already gone.

@jukzi
Copy link
Contributor Author

jukzi commented Jan 25, 2024

the code deleted is annotated with ancient

	 * @deprecated Please use {@link IServiceLocator#getService(Class)} instead.
	 *             This API is scheduled for deletion, see Bug 431177 for details
	 * @noreference IWorkbenchCommandSupport is scheduled for deletion.

@vogella
Copy link
Contributor

vogella commented Jan 25, 2024

Yes, this API can be deleted, I marked that for deletion a long time ago. Please move also the entry in https://help.eclipse.org/latest/index.jsp?topic=%2Forg.eclipse.platform.doc.isv%2Fporting%2Fremovals.html to the deleted API section.

@jukzi jukzi force-pushed the WorkbenchCommandSupport branch 2 times, most recently from b0f61b4 to c7a1e92 Compare January 26, 2024 08:04
jukzi pushed a commit to jukzi/eclipse.platform.releng.aggregator that referenced this pull request Jan 26, 2024
@jukzi
Copy link
Contributor Author

jukzi commented Jan 26, 2024

Please move also the entry

eclipse-platform/eclipse.platform.releng.aggregator#1748

jukzi pushed a commit to jukzi/eclipse.platform.releng.aggregator that referenced this pull request Jan 29, 2024
@jukzi
Copy link
Contributor Author

jukzi commented Jan 29, 2024

how to get rid of this errors?

09:25:24.894 [ERROR] Failed to execute goal org.eclipse.tycho:tycho-apitools-plugin:4.0.5-SNAPSHOT:verify (verify) on project org.eclipse.ui.workbench: There are API errors:
09:25:24.894 [ERROR] META-INF/MANIFEST.MF:0 The type org.eclipse.ui.commands.AbstractHandler has been removed from org.eclipse.ui.workbench_3.131.100
...
09:25:24.894 [ERROR] META-INF/MANIFEST.MF:0 The type org.eclipse.ui.commands.Priority has been removed from org.eclipse.ui.workbench_3.131.100
09:25:24.894 [ERROR] META-INF/MANIFEST.MF:5 The major version should be incremented in version 3.131.100, since API breakage occurred since version 3.131.0

@laeubi
Copy link
Contributor

laeubi commented Jan 29, 2024

how to get rid of this errors?

Add proper compatibility filter or increment the major version ...

@vogella
Copy link
Contributor

vogella commented Jan 29, 2024

Add proper compatibility filter or increment the major version ...

Filter. Major version increase for planned API deletion was tried once (by me) and caused a storm of complaints. Therefore, the PMC decided that or planned API deletions we only increase the minor version and use API filter (quickfix) to hide the warnings from the API tools.

@jukzi
Copy link
Contributor Author

jukzi commented Jan 30, 2024

I added filter with quick fix. The error on mac is unrelated. The "new ecj warnings" are false positives (added API warnings).
Can i submit?

@laeubi
Copy link
Contributor

laeubi commented Jan 30, 2024

The "new ecj warnings" are false positives (added API warnings). Can i submit?

They are not "false positives" pleas either fix API warnings or suppress them as well.

@jukzi
Copy link
Contributor Author

jukzi commented Jan 30, 2024

They are not "false positives" pleas either fix API warnings or suppress them as well.

Which so called "new" warnings exactly do you think is related to this PR?
https://ci.eclipse.org/platform/job/eclipse.platform.ui/job/PR-1563/6/eclipse/new/

fix "overriding a synchronized method without being synchronized"
and several deprecation warnings
@jukzi jukzi merged commit e4b25da into eclipse-platform:master Jan 31, 2024
16 checks passed
jukzi pushed a commit to eclipse-platform/eclipse.platform.releng.aggregator that referenced this pull request Jan 31, 2024
jonahgraham added a commit to jonahgraham/cdt that referenced this pull request Feb 8, 2024
The class DiscoveredPathContainerPage used a number of APIs in
the Eclipse Platform
[removed](eclipse-platform/eclipse.platform.ui#1563) in
[Eclipse 4.31 release](https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/81e406456ffae2f82fe0bb244adfdc4121c9e463/eclipse.platform.common/bundles/org.eclipse.platform.doc.isv/porting/removals.html#L595).
This class, while API, has not be used, nor usable since the CDT 4
release as it was only for CDT 3.x style projects.
The class had been deprecated since 2010.

To mitigate against the possibility that someone may have a dependency
on this old class the minor version has been bumped so that version
range can have `,8.3)` as their upper version.

Fixes eclipse-cdt#700
jonahgraham added a commit to eclipse-cdt/cdt that referenced this pull request Feb 8, 2024
The class DiscoveredPathContainerPage used a number of APIs in
the Eclipse Platform
[removed](eclipse-platform/eclipse.platform.ui#1563) in
[Eclipse 4.31 release](https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/81e406456ffae2f82fe0bb244adfdc4121c9e463/eclipse.platform.common/bundles/org.eclipse.platform.doc.isv/porting/removals.html#L595).
This class, while API, has not be used, nor usable since the CDT 4
release as it was only for CDT 3.x style projects.
The class had been deprecated since 2010.

To mitigate against the possibility that someone may have a dependency
on this old class the minor version has been bumped so that version
range can have `,8.3)` as their upper version.

Fixes #700
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.

None yet

4 participants