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

Fix Spotless Eclipse workspace cleanup and OSGI lock #451

Merged
merged 8 commits into from Sep 17, 2019
Merged

Conversation

fvgh
Copy link
Member

@fvgh fvgh commented Aug 31, 2019

Fixes #447

  • Eclipse workspace cleanup uses shut-down hook for recursive file/directory removal
  • Unnecessary OSGI lock removed

Change log has been updated. Fix is back-ward compatible. Hence a re-build of the Eclipse formatters is not required.

@fvgh fvgh requested a review from nedtwigg August 31, 2019 13:02
@fvgh
Copy link
Member Author

fvgh commented Aug 31, 2019

@nedtwigg
Please provide a new spotless-eclipse-base. I'll provide afterwards the finalization of change logs and dependency lock files as part of this branch/PR (or a new one, as you prefer...).

Currently I use Gradle 4. Shall Provide an independent PR for Gradle 5 first?

@nedtwigg
Copy link
Member

nedtwigg commented Sep 3, 2019

Published, tagged, and pushed to this branch. Sorry for slow turnaround, I was confused by the 4.x / 5.x thing. Imo, updating the build for all of the _ext projects is not very important. If anybody wants to do it, I'm all for it, but it's easy to continue to use an older gradle for them. In the previous commit I just pushed, I centralized the build instructions for all _ext projects into BUILD_INSTRUCTIONS.md, and added info on which gradle versions we've been using for them, and how to get it. Feel free to revert or force-push over it, @fvgh, if you disagree.

@fvgh
Copy link
Member Author

fvgh commented Sep 5, 2019

@nedtwigg Thanks for the Eclipse Read-Me update. I am afraid, I was short at time and could not get my head around quickly for a clean solution. I would like to finalize the issues I assigned to myself, before providing an update of the build process.

Please have a look at my latest changes. As you an see, the fix is only applied on the latest version of the Eclipse based formatters. Do you think that's sufficient? Should this be explicitly stated in the change logs?

@nedtwigg
Copy link
Member

nedtwigg commented Sep 5, 2019

could not get my head around quickly for a clean solution. I would like to finalize the issues I assigned to myself, before providing an update of the build process.

Good call! Just some random thoughts on this issue:

  • I think the current state of the _ext build is 100% fine
  • It looks like gradle has deprecated the -b some-other-dir/build.gradle functionality. Our build is already slow, and adding the p2 resolve of every _ext build will make it really slow!
  • We could split _ext into its own project, but I think that complicates our workflow unnecessarily.
  • One interesting but ultimately bad idea I've had is if (git diff <thiscommit> vs <master> has any changes in _ext) then run _ext build. But even that wouldn't have caught this particular regression, and if some manual action is needed, might as well save time trying to automate and just have a manual checklist.
  • A slightly better idea is to checkin a gradle wrapper for the _ext directory, separate from the root gradle wrapper.

the fix is only applied on the latest version of the Eclipse based formatters. Do you think that's sufficient?

I do. It looks like we ought to be able to do the swap for every single lockfile without causing any problems, but it's a little scary to do that without a pretty big test matrix. I'd be okay with taking the small risk of bumping every single one, but I'm also fine with the approach you have taken.

Should this be explicitly stated in the change logs?

Yes, if it's only fixed for the 4.12.0 lockfiles, then we should say so.

Added change log information about Eclipse fix restriction to 4.12.
Added change log information about Eclipse fix restriction to 4.12.
@fvgh fvgh requested a review from nedtwigg September 10, 2019 05:50
@fvgh
Copy link
Member Author

fvgh commented Sep 10, 2019

@nedtwigg Please have a last look at my Changes log entry. Feel free to change directly if you see the need. Afterwards we can do a squash and merge.

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

Sorry for delay! I updated changelog wording a bit. This LGTM, but we should regular-merge, and not squash-merge. The ext-eclipse-base/3.2.1 tag is on one of the intermediate commits.

@nedtwigg
Copy link
Member

I'm gonna merge since this is good and ready to go! I'm planning on fixing a few issues this weekend, then releasing on Monday.

@nedtwigg nedtwigg merged commit 6179497 into master Sep 17, 2019
@nedtwigg nedtwigg deleted the issue_447 branch September 17, 2019 22:02
@nedtwigg
Copy link
Member

Published in 3.24.3

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.

Resource leak for maven plugin used with java eclipse formatter
2 participants