-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
Build fails on Travis for three easy-to-fix On AppVeyor it's more weird: All Python 3 tests fail. |
Yeah, I know why this is happening. Should have a fix shortly. |
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:
Then Cookiecutter should throw an explicit error called something like: |
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?) |
There's a hiccup to raising an error on an unsupported platform if we see a symlink: the Would this PR work for you if we add:
In my mind, the justification for symlink support in 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 My preference in is the middle-of-the-road option, but I can see justification for the more conservative/aggressive approaches as well. |
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 :-( |
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. |
I think it would help to outsource the We could then simply go ahead with this PR, and when the compatibility project is ready we simply change the imports from |
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. |
Oooops, this project seems to already exist! 🎉
Found on StackOverflow for os.path.islink on windows with python. Patching the existing functions in |
@bittner As I wrote in #868, the existing @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 @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 |
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). |
@pjbull OK, I'm somewhat confused here. AIUI, cookiecutter creates a directory from a template that's copied to the user's machine in So how would I run cookiecutter on a template that contains symlinks? I don't think I can even create one... FWIW,
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. |
@pfmoore Changing your git config enables symlinks on Windows 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 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. |
@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. |
@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 So, just to make sure that we're on the same page after the chatter:
|
@pjbull Sounds reasonable to me. |
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. |
I'd be happy to see this merged anytime soon, as discussed above. 👍 |
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: |
Is this PR ready for merging? |
@pjbull Can you review the conflicting files? |
A project I'm working on would benefit from this functionality. Is there anything I can do to help with moving this forward? |
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. |
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! |
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. |
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). |
Looks like Python 2.7 support was removed for all future releases, so this PR can be simplified. |
- 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
@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. |
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. |
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! |
Closing as OP lost interest in driving it. We can reopen as I was supporting the change. |
@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? |
Sure, just keep it reporting green on CI and ping/poke a maintainer for review. We all struggle with available time... shortly: resolve conflicts. |
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? |
Hi @pjbull
After all done all tests should pass, coverage should be 100% and I will be glad to look on this. Thank you. |
Thanks for the detailed comments @insspb. Will do! |
@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. |
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.