--always-copy - do not use symlinks even when on supporting system

[m0she]

No description provided.

[pfmoore]

I don't really like the use of a global variable for this (and the fact that the travis build is failing demonstrates that using globals like this is error prone...) Ideally, can you fix the patch to avoid a global variable. But regardless, the build failures need to be fixed.

I'm not against the functionality, however.

[m0she]

Thanks for the prompt reply!

The problem with the travis build can be easily solved by initializing the variable to a default (False) value.
I've just now had the time to look into it more thoroughly. Since it is a configuration parameter that effects several non-related global functions, it should be accessible through some kind of global construct (singleton, global conf object, global variable). I'm not sure how you'd go about implementing this feature otherwise (without a massive refactor, like making an abstract interface for handling files, or objectifying the entire code).

What do you say?

[pfmoore]

My point is really that the fact that you forgot to initialise the global demonstrates that coding with a global variable like this introduces the risk of errors.

I'd prefer it if the option was passed around as a local variable via function arguments, the same way as options like use_setuptools or search_dirs. The key advantage is that the logic is explicit then.

I had a look at your patch with the intention of fixing it myself. But I found that the "action at a distance" involved in setting the always_copy flag meant that I very quickly lost track of what the code was trying to do. Between the always_copy global, the (very similar) symlink argument of copyfile and the (already present) confusion between use of copyfile and shutil.copyfile, the code is close to unmaintainable.

There doesn't seem to be an acceptable easy fix - I don't want to apply a quick hack, I'd rather the maintainability of the code was improved (or at least got no worse) as part of this patch. If you can refactor the code with that in mind, that would be great - otherwise, I'll look at doing so myself. But I won't have time to take a proper look at it for a while yet. Ping me in a few weeks if there's been no change on this.

One thing that would help put this in perspective is if you could explain the benefits of the change. Why would you even care whether symlinks or copies were used? (BTW, I do 99% of my development on Windows, so I don't really have a good feel for the pros and cons of symlinks - if it's obvious to a Unix person why you'd care then great, but I'd still appreciate some clarification for my own interest). If it's causing a significant issue, I'll see if I can find some time sooner.

[pfmoore]

See pull request #409 for an alternative implementation without a global variable. Please test this - I can't as I work on Windows and symlinks are not used there.

Also, your pull request does not change fix_lib64() which also uses symlink. Please review why your pull does not touch this function and whether or not it is needed.

Without some feedback from other Unix users (and in particular users for whom fix_lib64() is relevant) on the usefulness and correctness of one of these two patches, I'm going to leave this and #409 pending, as I don't have sufficient confidence that this won't break things for someone. (If any of the other devs with access to Unix want to apply this, I'm fine with that, obviously).

[m0she]

I really appreciate your effort but I couldn't get to it yet.
It's on my queue, hope to test it soon!

Thanks!

[pfmoore]

It's OK. pull #409 has now been merged. Closing this issue as fixed.