Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Make bundle clean clean extension directories #6168

Merged
merged 1 commit into from
Jan 10, 2018

Conversation

akhramov
Copy link
Contributor

What was the end-user problem that led to this PR?

The problem was that bundle clean command doesn't remove gem extensions (#5596)

What was your diagnosis of the problem?

I've looked into Bundler::Runtime#clean and realized that extension dirs are not removed

What is your fix for the problem, implemented in this PR?

My fix is to tweak Bundler::Runtime#clean to remove extensions dirs
as well.

Why did you choose this fix out of the possible options?

I chose this fix because I didn't see any other option.

@segiddins segiddins requested a review from indirect November 12, 2017 01:38
@segiddins
Copy link
Member

Thanks!

@colby-swandale
Copy link
Member

ping @indirect

@akhramov akhramov force-pushed the fix/clean-extensions branch 2 times, most recently from 05483a7 to b016599 Compare December 31, 2017 09:56
@@ -163,13 +163,15 @@ def clean(dry_run = false)
gem_dirs = Dir["#{Gem.dir}/gems/*"]
gem_files = Dir["#{Gem.dir}/cache/*.gem"]
gemspec_files = Dir["#{Gem.dir}/specifications/*.gemspec"]
extension_dirs = Dir["#{Gem.dir}/extensions/*/*/*"]
Copy link
Member

Choose a reason for hiding this comment

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

Could the glob pattern be "select directories recursively"?

Such as: Dir["#{Gem.dir}/extensions/*/*/*"] => Dir["#{Gem.dir}/extensions/**/*"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need exactly three levels of nesting. **/* will match the first level as well, i.e

Dir["#{Gem.dir}/extensions/**/*"]

Will match

extensions/x86_64-darwin-17
extensions/x86_64-darwin-17/2.4.0-static
extensions/x86_64-darwin-17/2.4.0-static/very_simple_binary-1.0
extensions/x86_64-darwin-17/2.4.0-static/very_simple_binary-1.0/gem_make.out
extensions/x86_64-darwin-17/2.4.0-static/very_simple_binary-1.0/gem.build_complete
extensions/x86_64-darwin-17/2.4.0-static/very_simple_binary-1.0/very_simple_binary_c.bundle
extensions/x86_64-darwin-17/2.4.0-static/simple_binary-1.0
extensions/x86_64-darwin-17/2.4.0-static/simple_binary-1.0/simple_binary_c.bundle
extensions/x86_64-darwin-17/2.4.0-static/simple_binary-1.0/gem_make.out
extensions/x86_64-darwin-17/2.4.0-static/simple_binary-1.0/gem.build_complete

Whereas

Dir["#{Gem.dir}/extensions/*/*/*"]

Will match

extensions/x86_64-darwin-17/2.4.0-static/very_simple_binary-1.0
extensions/x86_64-darwin-17/2.4.0-static/simple_binary-1.0

We don't want to remove extensions/x86_64-darwin-17, because this directory contains all extensions of all gems.

Please refer to following line for further details:
https://github.com/akhramov/bundler/blob/b016599152bddaf303618ca148ec59f623051893/lib/bundler/runtime.rb#L198

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the levels-explanation!

`bundle clean` command doesn't remove gem extensions, because
extensions reside out of gem directories.

This change tweaks `Bundler::Runtime#clean` to remove extensions dirs
as well.
@akhramov akhramov force-pushed the fix/clean-extensions branch from b016599 to f631eca Compare January 10, 2018 05:54
@akhramov
Copy link
Contributor Author

Rebased the branch to the latest master. Is there any way I can improve this PR?

@colby-swandale
Copy link
Member

Thanks! @bundlerbot r+

@bundlerbot
Copy link
Collaborator

📌 Commit f631eca has been approved by colby-swandale

@bundlerbot
Copy link
Collaborator

⌛ Testing commit f631eca with merge 2649066...

bundlerbot added a commit that referenced this pull request Jan 10, 2018
Make `bundle clean` clean extension directories

### What was the end-user problem that led to this PR?

The problem was that `bundle clean` command doesn't remove gem extensions (#5596)

### What was your diagnosis of the problem?

I've looked into `Bundler::Runtime#clean` and realized that extension dirs are not removed

### What is your fix for the problem, implemented in this PR?

My fix is to tweak `Bundler::Runtime#clean` to remove extensions dirs
as well.

### Why did you choose this fix out of the possible options?

I chose this fix because I didn't see any other option.
@bundlerbot
Copy link
Collaborator

☀️ Test successful - status-travis
Approved by: colby-swandale
Pushing 2649066 to master...

@bundlerbot bundlerbot merged commit f631eca into rubygems:master Jan 10, 2018
@colby-swandale colby-swandale added this to the 1.16.2 milestone Feb 4, 2018
@colby-swandale colby-swandale modified the milestones: 1.16.2, 1.17.0 Apr 4, 2018
colby-swandale pushed a commit that referenced this pull request Sep 20, 2018
Make `bundle clean` clean extension directories

### What was the end-user problem that led to this PR?

The problem was that `bundle clean` command doesn't remove gem extensions (#5596)

### What was your diagnosis of the problem?

I've looked into `Bundler::Runtime#clean` and realized that extension dirs are not removed

### What is your fix for the problem, implemented in this PR?

My fix is to tweak `Bundler::Runtime#clean` to remove extensions dirs
as well.

### Why did you choose this fix out of the possible options?

I chose this fix because I didn't see any other option.

(cherry picked from commit 2649066)
colby-swandale pushed a commit that referenced this pull request Oct 5, 2018
Make `bundle clean` clean extension directories

### What was the end-user problem that led to this PR?

The problem was that `bundle clean` command doesn't remove gem extensions (#5596)

### What was your diagnosis of the problem?

I've looked into `Bundler::Runtime#clean` and realized that extension dirs are not removed

### What is your fix for the problem, implemented in this PR?

My fix is to tweak `Bundler::Runtime#clean` to remove extensions dirs
as well.

### Why did you choose this fix out of the possible options?

I chose this fix because I didn't see any other option.

(cherry picked from commit 2649066)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants