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 broken behaviour in --excludes-file option #982

Merged
merged 3 commits into from Nov 7, 2015

Conversation

@wyaeld
Copy link
Contributor

wyaeld commented Aug 18, 2015

--exclude-file doesn't work due to a variable error on L367

@wyaeld

This comment has been minimized.

Copy link
Contributor Author

wyaeld commented Aug 18, 2015

hmm, fixing the typo is just resulting in further problems, so this fix isn't complete yes

wyaeld added 2 commits Aug 18, 2015
…. excludes itself resolves to nil at this point
This was necessary because I was getting world writable complaints on /tmp
even though it had 1777 permissions to start with.
@wyaeld

This comment has been minimized.

Copy link
Contributor Author

wyaeld commented Aug 18, 2015

Now its working.

exclude-file implementation was always broken, @ahammond missed one of the 3 variables in a88027e

exclude-file was trying to set the attributes on the exclude array from https://github.com/jordansissel/fpm/blob/master/lib/fpm/command.rb#L138 which @jordansissel introduced in cfee4c7 but it doesn't seem like later methods in the class can reference that, because its nil when exclude-file tries. Maybe that only works if both exclude and exlude-file are used together? I could use some advice

I changed path removal from remove_entry_secure to rm_r
Have been running into what looks like the same problem the nanoc guys faced here, nanoc/nanoc@aeb7115 so I borrowed their solution.

In my case /tmp was definitely modded to 1777 but was still hitting the problem.

@wyaeld wyaeld changed the title fix typo on local variable Fix broken behaviour in --excludes-file option Aug 18, 2015
@wyaeld

This comment has been minimized.

Copy link
Contributor Author

wyaeld commented Aug 18, 2015

A few other notes.

Path globbing with File.fnmatch isn't very intuitive, and is quite different from tar/git/docker

Here is a working excludes file with a few combinations

**.git
**.vagrant
**dist
**docs
**archive/**
**input/**
**output/**
**run/**
**etc/results.conf

You can ignore entire dirs, contents of dirs, or individual files still, but particularly if you've used the --directories option, then its going to be path patching based on the OUTPUT file structure, not the source structure, so that gets confusing, and probably why someone trying to exclude .git gets nowhere

@wyaeld

This comment has been minimized.

Copy link
Contributor Author

wyaeld commented Aug 18, 2015

oh yeah, and if you try to exclude tmp, everything dies

@wyaeld

This comment has been minimized.

Copy link
Contributor Author

wyaeld commented Sep 19, 2015

@jordansissel is this just dead now? Its a pain having to run all our packaging off a fork if no-one is going to look at the fix

@jordansissel

This comment has been minimized.

Copy link
Owner

jordansissel commented Sep 19, 2015

Its a pain having to run all our packaging off a fork

Sorry for your frustration.

if no-one is going to look at the fix

I haven't worked on fpm lately because of a lack of energy and lack of time to do so. In the meantime, I invite anyone else to review + test this fix for style, backwards-compatibility, added tests, etc. If someone else reviews this and says it works well and doesn't break anything else, I'll be happy to click the merge button.

@imoore76

This comment has been minimized.

Copy link

imoore76 commented Oct 18, 2015

I can vouch for it. This works well and the merge would be very much appreciated! We're using fabric to launch fpm, which won't pass multiple flags with duplicate names. E g multiple --exclude flags. A single --exclude-file does the trick though

@jordansissel

This comment has been minimized.

Copy link
Owner

jordansissel commented Nov 7, 2015

@imoore76 thaks for the confirmation!

@wyaeld thank you for your efforts here. LGTM! :)

jordansissel added a commit that referenced this pull request Nov 7, 2015
Fix broken behaviour in --excludes-file option
@jordansissel jordansissel merged commit 32ce7f6 into jordansissel:master Nov 7, 2015
jordansissel added a commit that referenced this pull request Jun 20, 2016
Fix broken behaviour in --excludes-file option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.