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

Add support for symlinks #934

Closed
wants to merge 8 commits into from
Closed

Conversation

pjbull
Copy link
Contributor

@pjbull pjbull commented Apr 23, 2017

This is in place of PR #868, which also include fixes for Windows that seem too messy to be worth pulling in to maintain.

Caveat: Symlinks will not work as expected on Windows with Python versions < 3.2 (but, they don't now anyway...)

Updated PR to support symlinks in templates across platforms now that we don't support older versions of Python.

@bittner
Copy link
Contributor

bittner commented Apr 23, 2017

Build fails on Travis for three easy-to-fix flake8 complaints in tests/test_generate_symlinks.py.

On AppVeyor it's more weird: All Python 3 tests fail.

@pjbull
Copy link
Contributor Author

pjbull commented Apr 23, 2017

Yeah, I know why this is happening. Should have a fix shortly.

@pydanny
Copy link
Member

pydanny commented Apr 24, 2017

I like that this PR is simpler than #868. On the other hand, it's always tricky when we have a feature that works in one operating system and not another that doesn't throw warnings or exceptions to users. Considering our supported versions of Python in https://github.com/audreyr/cookiecutter/blob/master/setup.py, how about this:

If:

  • on Windows
  • running Python 3.2 or lower
  • symlinks are attempted

Then Cookiecutter should throw an explicit error called something like: NoSymlinksOnWindowsPython27 and NoSymlinksOnWindowsPython32, followed by a concise but understandable explanation. That will allow us (the maintainers) to have an easy resolution when someone files an issue for a Cookiecutter template taking advantage of symlinking.

@pydanny
Copy link
Member

pydanny commented Apr 24, 2017

Also, @pjbull and @bittner, is there any reason why you want Cookiecutter to handle symlinks in the templating section? Why not just write a post gen hook? Seems to me that the better way to do things - you certainly have a lot more fine-grained control!

@bittner
Copy link
Contributor

bittner commented Apr 25, 2017

I'm not sure I understand @pydanny's intent in "why you want Cookiecutter to handle symlinks in the templating section".

I assume reproducing a cookiecutter template "as is" is meant to be the default behavior of the Cookiecutter tool. Why exclude symlinks? Isn't the current behavior a bug? (Imagine a relative symlink pointing to the current version of, say, a logfile or the active configurations of Apache.)

Cookiecutter should reproduce. Period.

It should be the responsibility of the cookiecutter template author to ensure the cookiecutter (template, not the tool) works across different environments and operating systems (I commented on this in PR #868 already). If there is a symlink in a cookiecutter I should know as an author that this may only work on Unix-like OSes. Cookiecutter should create a symlink, not a copy of real file, if I put a symlink under version control.

(Interesting, it looks like symlinks are supported on Windows since Vista, but has anyone seen anyone using that feature?)

@pjbull
Copy link
Contributor Author

pjbull commented Apr 25, 2017

There's a hiccup to raising an error on an unsupported platform if we see a symlink: the islink function doesn't work, so we'll never know that we're in an unsupported case. I think the best we can do is have a warning on the platform, and maybe in the documentation.

Would this PR work for you if we add:

  • warning in main.py if symlinks are not supported (this would always pop-up, regardless of if symlinks are attempted)
  • Add documentation about symlink support to the docs (including which OS/Python combos are supported)

In my mind, the justification for symlink support in cookiecutter is that it is a filesystem feature that some people just expect to work (right now it fails silently). We see it more often in the data science use cases that we work on where you want to consistently point collaborators to a data source that's mounted elsewhere. It's not the only way to do that, but it is a reasonable way.

The more aggressive approach is to use the bigger PR in #868, trust in the tests, and support symlinks across platforms. Hopefully some day that all gets deprecated when official support goes to > Python 3.2 only.

The more conservative approach is just to have a warning if we see a symlink that symlinks are not supported and authors of templates should handle linking in a post gen hook.

My preference in is the middle-of-the-road option, but I can see justification for the more conservative/aggressive approaches as well.

@pfmoore
Copy link
Contributor

pfmoore commented Apr 25, 2017

warning in main.py if symlinks are not supported (this would always pop-up, regardless of if symlinks are attempted)

So you're suggesting that cookiecutter would warn every time it's used on Windows? Regardless of whether symlinks (a feature that's presumably not used in any existing cookiecutter) are used in the cookiecutter the user requests?

That seems pretty hostile to Windows users :-(

@pjbull
Copy link
Contributor Author

pjbull commented Apr 25, 2017

@pfmoore Yes, it would warn on Windows + Python < 3.2 (so, not all Windows instances).

I definitely don't want to be hostile to Windows users (for proof, see #868). I'm open to other solutions if you have a suggestion.

@pfmoore
Copy link
Contributor

pfmoore commented Apr 25, 2017

Just fail if the template tries to use a symlink on a platform that doesn't support them? You say islink doesn't work if symlinks aren't supported - I don't follow. If you git clone a cookiecutter with a symlink in it, what would you get on a platform without symlinks? I'd assume you'd get a warning/error from git that the clone didn't work as expected - why not catch that and report it to the user? You certainly wouldn't get a symlink in the checkout (It's worth noting that on Windows, even in Win 10, users can only create symlinks if they have a specific privilege - in practical terms, that means you're in an elevated prompt, which people won't be 99% of the time, so "you can't realistically use symlinks on Windows" is a pretty good rule of thumb).

If you have a public repo with symlinks, I'd be happy to test by doing a clone of that repo and reporting the results back here.

@bittner
Copy link
Contributor

bittner commented Apr 25, 2017

I think it would help to outsource the win_utils.py module to a compatibility project on PyPI that handles islink, readlink, symlink, copytree transparently. This can be managed with additional care outside the scope of this project.

We could then simply go ahead with this PR, and when the compatibility project is ready we simply change the imports from from os import ... and from os.path import ... to, say, from os_path_compat import ..., and we're done. (I would also ask @jaraco before creating that compatibility project, maybe he or someone else knows if such a project already exists.)

@pydanny
Copy link
Member

pydanny commented Apr 25, 2017

I think it would help to outsource the win_utils.py module to a compatibility project on PyPI that handles islink, readlink, symlink, copytree transparently. This can be managed with additional care outside the scope of this project.

That would be pretty awesome. I did something similar to that with https://github.com/pydanny/whichcraft, which was me pulling out the code of Cookiecutter so it could be easily tested independently. I know it seems like a tiny, silly package, but moving this stuff out means the problem becomes more shallow and easier for more people to contribute to fixing issues.

@bittner
Copy link
Contributor

bittner commented Apr 25, 2017

Oooops, this project seems to already exist! 🎉

Found on StackOverflow for os.path.islink on windows with python.

Patching the existing functions in os, as described in the README, looks promising.

@pjbull
Copy link
Contributor Author

pjbull commented Apr 25, 2017

@bittner As I wrote in #868, the existing jaraco.windows functions do not work as expected. If you just use those imports, the symlink tests fail. There is an error when rmtree tries to remove the directories. That's the reason win_utils got rolled in the first place. Before creating the posix-only PR, I spent a solid half day in the weeds on finding a workaround that used jaraco.windows and still passed the tests. I didn't get there, so would love more eyes in the problem. Here's a branch to work off of if you want to test that out:
https://github.com/pjbull/cookiecutter/tree/win-symlink-jaraco

@pfmoore Pretty sure that the default behavior is just to produce a text file with the path that is supposed to be linked to if you clone a git repo with symlinks. Here are their docs for that: https://github.com/git-for-windows/git/wiki/Symbolic-Links Also, not sure that relying on git errors/warnings would work anyway since there's no guarantee a template is cloned withgit, right? Also, here's a repo if you want to give it a shot: https://github.com/sschuberth/git-playground

@pydanny Yeah, the purpose of this new PR was just to extract the functionality that we can use now and then solve the Windows symlinks problem elsewhere. Maybe that's a new package, maybe there's a fix for a bug in jaraco.windows. Either way, I guess the question is what is the stop gap that lets posix folks use symlinks while the Windows work is in progress? Is it a warning? Is it a docs change? Happy to do whatever unblocks those concerns so this could be merged.

@bittner
Copy link
Contributor

bittner commented Apr 25, 2017

For me merging this PR is fine, my use case is only affected by a single line though, to be honest.

shutil.copytree(indir, outdir, symlinks=True)

My problem, if that is not yet clear, is that Cookiecutter does not create what I put under version control. A postgen hook would be pointless (makes a special case that needs postprocessing for every single symlink put under version control, or requires moving file system content into postgen hook business logic, if you want so).

@pfmoore
Copy link
Contributor

pfmoore commented Apr 25, 2017

@pjbull OK, I'm somewhat confused here. AIUI, cookiecutter creates a directory from a template that's copied to the user's machine in ~/.cookiecutters. If I'm not able to create symlinks (non-elevated, no special privs enables) then that template won't have symlinks in there. So by the time we're creating the target directory from the template it's already too late.

So how would I run cookiecutter on a template that contains symlinks? I don't think I can even create one...

FWIW, git clone https://github.com/sschuberth/git-playground on my PC gives no errors, and creates a directory ````linux-links``` with 3 text files:

>dir

    Directory: C:\Work\Scratch\git-playground\linux-links

Mode                LastWriteTime         Length Name
----                -------------         ------ ----
-a----       25/04/2017     22:52             14 this_is_a_hard_link_to_file
-a----       25/04/2017     22:52             14 this_is_a_regular_file
-a----       25/04/2017     22:52             22 this_is_a_symbolic_link_to_file

>type .\this_is_a_hard_link_to_file
Hello world!
>type .\this_is_a_regular_file
Hello world!
>type .\this_is_a_symbolic_link_to_file
this_is_a_regular_file

Anyway, this is pretty off-topic. My only point was that I don't think it's reasonable for cookiecutter to always warn the user that symlinks aren't supported, without even checking if it matters for what the user's actually doing.

@pjbull
Copy link
Contributor Author

pjbull commented Apr 25, 2017

@pfmoore Changing your git config enables symlinks on Windows git config --global core.symlinks true, so you can have a template with symlinks that you cloned from a git repository (which is what this PR does on Appveyor). Also, for the record, Python supports creating, reading, and testing for symlinks on Windows in versions >= 3.2, so it's not crazy to think users could have symlinks on their Windows systems.

Agreed that it is not ideal to have super chatty warnings about symlinks if users aren't using them. My only point is if a user cloned a git repo that did have symlinks by changing their config, we still can't warn in cookiecutter because islink will be False for those links when Python < 3.2 (even though they are actually symlinks on the filesystem).

I'd like to do the right thing for Windows users and posix users. Unfortunately, it just doesn't seem like there's an easy Windows + Py3.2 fix at the moment. My current vote would be to add a section in the docs about symlinks that includes information on the git config and the fact that they won't work on Windows systems with Python < 3.2. Happy to write that up if that's the preferred fix.

@pfmoore
Copy link
Contributor

pfmoore commented Apr 25, 2017

@pjbull My point is that you don't just need the git setting, you also need some fairly annoying to set privileges. But yes, if you have set things up so you can use symlinks, why not have them work?

If it's just Python 3.2 that's the problem (sorry, hadn't spotted that) then it's not a big deal. Very few people even bother supporting 3.2 any more, so limited functionality seems reasonable in that case. One thought - would it be possible to patch the symlink functions to immediately give an error on unsupported systems? Then you could ensure that you fail if the codepath that uses symlinks is taken, without affecting other codepaths.

But a doc section saying "unsupported on Windows with Python < 3.2" seems like a sufficient approach.

@pjbull
Copy link
Contributor Author

pjbull commented Apr 26, 2017

@pfmoore It's actually all Python versions < 3.2 (including all of Python 2, including 2.7). I probably had admin privileges on my Azure VMs, but I never needed any changes beyond the git config to use symlinks in my testing. Not entirely sure why...

I think we can patch readlink/symlink if we also take the jaraco.windows dependency and use the islink function from there (otherwise islink is always false and we'll never hit read or create). That may actually be good since it makes it easier for someone to figure out the bugs that were preventing jaraco.windows just working for our use case (as mentioned above).

So, just to make sure that we're on the same page after the chatter:

  • Docs update with specific info about symlinks
  • On Windows + Py < 3.2, use the jaraco.windows version of islink. Patch readlink and symlink to raise NoSymlinksOnWindowsPython27 with a clear message that symlinks are not supported in that configuration.
  • Add bug for fixing symlinks on Windows + Python < 3.2 (the fix may ultimately be in jaraco.windows, but the current failure I see is in our rmtree function so it needs a little more investigation) and dump all this info in there.

@pfmoore
Copy link
Contributor

pfmoore commented Apr 26, 2017

@pjbull Sounds reasonable to me.

@jaraco
Copy link

jaraco commented Apr 26, 2017

the existing jaraco.windows functions do not work as expected. ... There is an error when rmtree tries to remove the directories.

I'd be delighted to see a bug report and if possible a PR to test and fix this issue with jaraco.windows. I'll get it set up on AppVeyor for CI support.

@bittner
Copy link
Contributor

bittner commented May 16, 2017

I'd be happy to see this merged anytime soon, as discussed above. 👍

@pjbull
Copy link
Contributor Author

pjbull commented May 16, 2017

I've completed all the tasks above including docs and a new issue. (Sorry for delay, I had to find some time and it usually takes longer to spin up on this since I don't have a Windows box).

#947 now tracks implementing symlinks for Windows + Python < 3.2. I'll update that bug with a couple of relevant links once this is merged.

I'll also close the following:

@bittner
Copy link
Contributor

bittner commented Jun 2, 2017

Is this PR ready for merging?

@theodesp
Copy link

theodesp commented Aug 9, 2017

@pjbull Can you review the conflicting files?

@michaeljoseph michaeljoseph self-requested a review August 9, 2017 21:21
@milancermak
Copy link

A project I'm working on would benefit from this functionality. Is there anything I can do to help with moving this forward?

@pydanny
Copy link
Member

pydanny commented May 2, 2018

Yes, you can expedite the review of this and other pull requests by getting your employer to increase our funding:

For reference, Cookiecutter works in a stable manner on all operating systems and supported versions of Python. Our plan is to maintain the status quo moving forward unless we can get more funding. Adding more features increases the workload of the maintainers, who are fundamentally unpaid for working on this project.

@milancermak
Copy link

All clear. I've became a small patreon myself, will talk to the folks at my company to see if we can increase that. Thanks to all of you!

@simobasso simobasso linked an issue May 15, 2021 that may be closed by this pull request
@samschott
Copy link

Is there any interest on picking this up again? Symlinks can be an essential part of a project structure in Unix and being able to preserve those from a cookiecutter template would be great.

One example use case are templates which contain macOS Frameworks.

@bittner
Copy link
Contributor

bittner commented Jul 7, 2021

Is there any interest on picking this up again? Symlinks can be an essential part of a project structure in Unix and being able to preserve those from a cookiecutter template would be great.

This issue should definitely be picked up again. IMHO, fixing this bug is not about taste or personal preference. A file system-based template should reproduce the exact original state that it was drafted from.

The fact that symlinks need to be handled in a special manner on Mircosoft Windos should be regarded as a technical detail, not a go/no-go style decision. It could be handled as a (temporarily) unsupported feature to start with (as suggested by @pydanny above).

@pjbull
Copy link
Contributor Author

pjbull commented Jul 7, 2021

What's the end-of-life story for Python 2.7 and cookiecutter? IIRC, once support for 2.7 is officially dropped, this PR can be simplified to remove any of the Windows-specific pieces since the standard library supports everything we need.

That said, even on 2.7 this PR improves the experience on Windows over the current behavior.

Looks like Python 2.7 support was removed for all future releases, so this PR can be simplified.
#1386

 - Support copying symlinks as symlinks for both rendered and
non-rendered directories
 - Support symlinks that link to a rendered directory
 - Support symlinks that get rendered
@pjbull pjbull changed the title Add support for symlinks on posix systems Add support for symlinks Jul 8, 2021
@pjbull
Copy link
Contributor Author

pjbull commented Jul 8, 2021

@ssbarnea PR is updated and tests are passing except one complexity check in code climate. I don't think the complexity is too drastic, and it is a straightforward way to support symlinks without changing any core existing logic, so I think it may be worth an exception. I could look at a refactor if that is preferable.

@ssbarnea ssbarnea added the feature This issue/PR relates to major feature request. label Jul 9, 2021
@ssbarnea ssbarnea requested a review from pydanny July 9, 2021 10:39
@ssbarnea
Copy link
Member

ssbarnea commented Jul 9, 2021

That is one of the oldest PRs I ever seen and getting it to green shows a lot of dedication. I approved it but with the side note that my review was not really extensive, so please I ask at least another core to review it before making a decision, nobody likes regressions.

@pjbull
Copy link
Contributor Author

pjbull commented Jul 9, 2021

so please I ask at least another core to review it before making a decision, nobody likes regressions.

Agreed, I was leaning on the test cases that I wrote way back when, so if there are ideas for scenarios that need more comprehensive tests, I would be happy to write them!

@ssbarnea
Copy link
Member

Closing as OP lost interest in driving it. We can reopen as I was supporting the change.

@ssbarnea ssbarnea closed this Mar 18, 2022
@pjbull
Copy link
Contributor Author

pjbull commented Mar 18, 2022

@ssbarnea Sorry, I thought this was waiting for review from another maintainer. Is there something else I can do to support this change getting merged?

@ssbarnea
Copy link
Member

ssbarnea commented Mar 19, 2022

Sure, just keep it reporting green on CI and ping/poke a maintainer for review. We all struggle with available time...

shortly: resolve conflicts.

@pjbull
Copy link
Contributor Author

pjbull commented Jun 7, 2022

Just made some updates to this branch to resolve conflicts. Could this PR be re-opened (looks like I don't have permissions for that), or should I start a new one?

@insspb
Copy link
Member

insspb commented Jun 7, 2022

Hi @pjbull
Thank you for interest. this PR cannot be reopened because it was force pushed. So you need to open new one.
Looks like big feature and a lot of discussion here. As I was thinking about some different problems, to speed up review of this please make some more changes

  • Move everything new from os.path to pathlib, as it is what considered next huge merge (before this)
  • Link to this discussion, so I can reread all of this
  • Update docs (we are supporting python 3.7 and up, I see 3.2 there
  • Update logger statements to logger.debug("message {}", var_in_message)
  • remove from __future__ if not required
  • extend functions :param variable: definition in functions where you declare variables. If function does not contain :param for other variables, please add them too

After all done all tests should pass, coverage should be 100% and I will be glad to look on this.
I am very interested to test it on windows, where I will be in few days.

Thank you.

@pjbull
Copy link
Contributor Author

pjbull commented Jun 7, 2022

Thanks for the detailed comments @insspb. Will do!

@insspb
Copy link
Member

insspb commented Jun 7, 2022

@pjbull You are welcome. I recommend to start from everything except moving to pathlib, and do pathlib move after other codebase will be updated. In this case it will be much more easy to you how to move this in each case of functionality usage. It will take few days to wait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to major feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question: Symbolic links