More OS friendly fix for the clean paths issue #572

Merged
merged 0 commits into from Oct 17, 2012

Projects

None yet

3 participants

@croach

Hi Fabio,

I was looking over the fix you added for the clean paths issue that I brought up in Issue #568 and I'm not totally sure that forcing a lower case on the directories is the right way to go about fixing the problem.

The problem as I see it is an inconsistency between the paths returned from a call to Dir.glob in the clean_paths method and a similar call to Pathname.glob in the expanded_paths method. In the latter, you add the File::FNM_CASEFOLD flag which is causing ruby to change the Developer directory name to lower case. To fix this you're lower casing the pathname by hand in the clean_paths method, but it would be better to let the OS work out the case of the directories on its own. So, a better fix would be to either add the File::FNM_CASEFOLD flag to the call to Dir.glob in the clean_paths method or remove it from the call to Pathname.glob in the expanded_paths method. I would suggest the latter since, according to the ruby docs, case insensitivity is an OS specific thing, so the File::FNM_CASEFOLD flag is ignored by the glob method. By this reasoning, you shouldn't use the File::FNM_CASEFOLD flag in your call to glob.

This pull request implements the fix I specify above. I've tested it out on my system and it worked for me, and I think this should the fix the problem in a way that will have the least amount of unforeseen side effects.

Hope this helps. Let me know if you have any questions.

@fabiopelosin
CocoaPods member

I was looking over the fix you added for the clean paths issue that I brought up in Issue #568 and I'm not totally sure that forcing a lower case on the directories is the right way to go about fixing the problem.

To simplify default patterns a bit CocoaPods treats the sources of Pods case insensitively (see aa1bea9). CocoaPods is intended to be used mostly on Mac OS X and for this reason we don't expect libraries with case sensitivity. We treat only that case insensitively, and this behavior would simplify a lot of things if we start to follow a more convention over configuration approach (more default patterns). With case sensitivity only matching the default license could be much more complex.

Licence
License
LICENSE
LICENCE
license
licence

Also currently the license globbing doesn't match others common names like MIT-License.

The problem as I see it is an inconsistency between the paths returned from a call to Dir.glob in the clean_paths method and a similar call to Pathname.glob in the expanded_paths method. In the latter, you add the File::FNM_CASEFOLD flag which is causing ruby to change the Developer directory name to lower case.

I suspected something like that, Thanks for the awesome investigative work. I still do have an unanswered question though, why is it changing the case of only the Developer directory and not the rest of the root? No clue.

I would suggest the latter since, according to the ruby docs, case insensitivity is an OS specific thing, so the File::FNM_CASEFOLD flag is ignored by the glob method. By this reasoning, you shouldn't use the File::FNM_CASEFOLD flag in your call to glob.

Note that case sensitivity depends on your system (so File::FNM_CASEFOLD is ignored), as does the order in which the results are returned.

I totally missed that note. We could also do something like:

  def clean_paths
      files = root.glob("**/*", File::FNM_CASEFOLD, File::FNM_DOTMATCH)

      files.reject! do |candidate|
        candidate.end_with?('.', '..') || used_files.any? do |path|
          path.include?(candidate) || candidate.include?(path)
        end
      end
      files
    end

Btw, removing the used = used_files.map(&:downcase) you are forcing a pattern match on every iteration of the any? call because used_files is not cached and the methods that it calls in turn are not cached.


So what do you think?

@croach
@fabiopelosin fabiopelosin was assigned Oct 10, 2012
@fabiopelosin
CocoaPods member

Awesome investigative work. It really looks like a bug in Mac OS X.

Are you going to update your pull request? If you do please document somewhere in the LocalPod file the explanation of the issue.

For the record, my previous commit (8e72cbe) creates other side effects as the downcased paths generate problems in other parts like the header_mappings (from.relative_path_from(root) is not case insensitive and as the root preserves cases and from is downcased it returns the wrong path).

@croach croach merged commit fe96a97 into CocoaPods:master Oct 17, 2012

1 check passed

Details default The Travis build passed
@croach croach referenced this pull request Oct 17, 2012
Merged

Clean paths fix #602

@croach

Sorry, it took me so long to get around to updating the pull request. Since, this pull request was made off of master instead of a feature branch (my mistake), I decided to close it and replace it with another made from a proper feature branch. This should make any further updates much easier and neater. The new pull request contains the updates discussed above and can be found here: #602.

@fabiopelosin
CocoaPods member

Thanks for the pull, I'll review it soon.

@alloy
CocoaPods member

I tried to catch up with this thread, but bear with me if I overlooked some info :)

@croach Do you by any chance have a case-sensitive filesystem?

@croach
@alloy
CocoaPods member

Moving discussion to #602.

@fabiopelosin fabiopelosin added a commit that referenced this pull request Oct 19, 2012
@fabiopelosin fabiopelosin [LocalPod] Always use Pathname::glob.
This patch also removes the custom Pathname.glob based on Dir::glob.

Related #572 and #602.
148371c
@jonmorehouse jonmorehouse pushed a commit that referenced this pull request Feb 22, 2014
@croach croach Updated `clean_paths` and `expand_paths` methods to use `File::FNM_CA…
…SEFOLD`

This change is an update to an earlier pull request to fix Issue
#572.
fa7c982
@kylef kylef pushed a commit that referenced this pull request Oct 25, 2014
@croach croach Updated `clean_paths` and `expand_paths` methods to use `File::FNM_CA…
…SEFOLD`

This change is an update to an earlier pull request to fix Issue
#572.
6e60c43
@fabiopelosin fabiopelosin added a commit that referenced this pull request Oct 25, 2014
@fabiopelosin fabiopelosin [LocalPod] Always use Pathname::glob.
This patch also removes the custom Pathname.glob based on Dir::glob.

Related #572 and #602.
ffc8edb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment