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 "Make LaunchManager's IResourceDeltaVisitors more light weight" #1616

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

trancexpress
Copy link
Contributor

This reverts commit 8d5bfec.

Fixes: #1609

Copy link
Member

@iloveeclipse iloveeclipse left a comment

Choose a reason for hiding this comment

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

Please merge onve tests complete

Copy link
Contributor

Test Results

 1 755 files  ±0   1 755 suites  ±0   1h 41m 40s ⏱️ + 11m 45s
 4 170 tests ±0   4 148 ✅ ±0   22 💤 ±0  0 ❌ ±0 
13 107 runs  ±0  12 943 ✅ ±0  164 💤 ±0  0 ❌ ±0 

Results for commit 74aac75. ± Comparison against base commit 0abe5a5.

@trancexpress trancexpress merged commit b4bff60 into eclipse-platform:master Nov 13, 2024
16 of 17 checks passed
@merks
Copy link
Contributor

merks commented Nov 13, 2024

I was going to suggest that it might be polite to given @HannesWell a chance to comment/review, but that's water under the bridge now...

@HannesWell
Copy link
Member

I was going to suggest that it might be polite to given @HannesWell a chance to comment/review, but that's water under the bridge now...

Yes. But I would have appreciated it even more if instead of doing a full revert just the parts that are not trivial would have been reverted. When you are already familiar with the reproducer it will probably relatively easy to filter out the change that caused the regression. Now both of us have to setup a reproducer.

@iloveeclipse
Copy link
Member

The constraints we had:

  • we wanted to fix this regression for RC1
  • the original change was not a significant feature that would be worth to wait for RC2 but a refactoring
  • pocking around in a code that was seemingly "trivial" but had a subtle bug could cause another regression
  • we had no time & recources (I'm offline)
  • we had no guarantee Hannes will be there with a fix before RC2

Under these conditions doing a revert was the safest approach.

@trancexpress
Copy link
Contributor Author

Yes. But I would have appreciated it even more if instead of doing a full revert just the parts that are not trivial would have been reverted. When you are already familiar with the reproducer it will probably relatively easy to filter out the change that caused the regression. Now both of us have to setup a reproducer.

In future, please separate clean-ups from actual changes (with different commits). I would have reverted only the actual changes that cause the bug, if this was the case.

I checked the diff but had no time to divine what actually changed.

@HannesWell
Copy link
Member

Under these conditions doing a revert was the safest approach.

Ok, that's understandable, it would just have been nice to have, anyways. Thanks for fixing that issue.
I have now created a second PR with split commits:

@trancexpress if you still have the reproducer at hand, it would be nice if you would have a look. If not I'll check if myself.

Yes. But I would have appreciated it even more if instead of doing a full revert just the parts that are not trivial would have been reverted. When you are already familiar with the reproducer it will probably relatively easy to filter out the change that caused the regression. Now both of us have to setup a reproducer.

In future, please separate clean-ups from actual changes (with different commits). I would have reverted only the actual changes that cause the bug, if this was the case.

Usually that's what I usually do. I assume in that case I assume that the change is so small and save. But I'm not 100% sure anymore since I did the change four month ago.

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.

[2024-09 regression] Restart is needed to refresh shared launch configurations
4 participants