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

Revert "Fix eclipse-platform/eclipse.platform#1173 - bad UI on MacOS merge viewer" #869

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

sratz
Copy link
Member

@sratz sratz commented Nov 20, 2023

Reverts #732

Async-exec caused a NPE, see #868.

Fixes #868.

Also, there are some concerns regarding the actual fix, see #732 (comment)

Copy link
Contributor

github-actions bot commented Nov 20, 2023

Test Results

     591 files  ±0       591 suites  ±0   1h 5m 11s ⏱️ + 5m 55s
  3 837 tests ±0    3 829 ✔️  - 3    5 💤 ±0  3 +3 
12 117 runs  ±0  12 078 ✔️  - 3  36 💤 ±0  3 +3 

For more details on these failures, see this check.

Results for commit 534c362. ± Comparison against base commit 078b338.

♻️ This comment has been updated with latest results.

@sratz
Copy link
Member Author

sratz commented Nov 22, 2023

@vogella @iloveeclipse would this revert be fine for RC2? What about the API tooling failures? They are again caused by eclipse-tycho/tycho#3019 but I don't think we should now touch all the other bundles here.

@HeikoKlare
Copy link
Contributor

With respect to the API tools failure: The builds for this PR should work once the configuration for org.eclipse.help.base is changed, see #862 and #871. It should not be necessary to adapt further configurations for the builds of this PR.

@vogella
Copy link
Contributor

vogella commented Nov 23, 2023

Fine for me to merge for rc2

@iloveeclipse
Copy link
Member

Fine for me to merge for rc2

No, it needs a workaround for API errors, otherwise it will most likely break the SDK build.

@HeikoKlare
Copy link
Contributor

#871 would fix all API errors. Or at least merging #862 should be sufficient for the builds of this PR to work again. The latter would probably be the "save" one to get this merged for the upcoming release.

@iloveeclipse
Copy link
Member

But that is not acceptable for rc2 as it would touch too many bundles

@HeikoKlare
Copy link
Contributor

#862 would only touch a single bundle. If it allows to merge this PR, maybe that's worth it? In particular because the only effect of that PR is that API tools will work for the affected bundle again.

But I only want to emphasize that as an opportunity to still merge this revert PR. I know it's very late in the release cycle and it's totally fine for me if we keep it as it is as changes are too large/risky.

@iloveeclipse
Copy link
Member

#862 would only touch a single bundle. If it allows to merge this PR, maybe that's worth it? In particular because the only effect of that PR is that API tools will work for the affected bundle again.

OK, that's good.

But I only want to emphasize that as an opportunity to still merge this revert PR. I know it's very late in the release cycle and it's totally fine for me if we keep it as it is as changes are too large/risky.

I think we should merge this revert after #862
I will do that now.

@iloveeclipse iloveeclipse merged commit 197f7db into master Nov 23, 2023
11 of 14 checks passed
@iloveeclipse iloveeclipse deleted the revert-732-badui branch November 23, 2023 22:04
@HeikoKlare
Copy link
Contributor

Great. The test failures are random failures as documented in #770.

@laeubi
Copy link
Contributor

laeubi commented Nov 24, 2023

#862 would only touch a single bundle.

I also wanted to note, that changing the build.properties here is not a bundle change as we do not include the file in the bundle and therefore a configuration change that can happen anytime in the release cycle (@akurtakov might correct me if I'm wrong) as long as the build works (or fix the build) and we do not change things like bin.includes (what have an impact on the bundle content).

@HeikoKlare
Copy link
Contributor

I also wanted to note, that changing the build.properties here is not a bundle change as we do not include the file in the bundle and therefore a configuration change that can happen anytime in the release cycle

That's also my understanding. But still one note on that: this kind of change to the build.properties seems to be treated as a change to the bundle by the API tools, which means that they will require a (micro) version bump. In case the bundle has not been touched in the current release cycle, the change to the build.properties will thus require a version bump to make API validation succeed, which then affects the generated artifact.
Maybe I misunderstood something, but when fixing all the build.properties for the platform repositories, I came across API tools complaining about missing version increments in case it was not incremented in the current release cycle already, see, e.g., eclipse-platform/eclipse.platform.ui#1321

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.

NPE in ContentMergeViewer
5 participants