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

let deploy generator install symlinks #7044

Merged
merged 4 commits into from May 19, 2020

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented May 18, 2020

Changelog: Bugfix: Fix deploy generator management of relative symlinks.
Docs: Omit

Close #7037

Supersedes #7036 also taking the #6994 to Conan 1.25.2

@memsharded memsharded added this to the 1.25.2 milestone May 18, 2020
@memsharded
Copy link
Member Author

@memsharded memsharded commented May 18, 2020

Hi @braindevices

I am trying to fix this for 1.25.2. I have added some more tests instead of modifying existing to check all behaviors are addressed. Also, I am unconditionally removing the dst file inside the is link logic. Please check if it will fit your case too.

if not os.path.isabs(link_target):
link_target = os.path.join(os.path.dirname(src), link_target)
linkto = os.path.relpath(link_target, os.path.dirname(src))
if os.path.isfile(dst):
Copy link
Contributor

@braindevices braindevices May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong. When dst is link already isfile return False, when dst is broken link exists return False too. We need to do os.path.exists(dst) or os.path.islink(dst)

Copy link
Member Author

@memsharded memsharded May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added it, together with more tests. Actually it was not necessary, because the os.symlink() actually works when it is a broken symlink. But I agree it is more evident to explicitly remove it here. Please check again, thanks!

Copy link
Contributor

@braindevices braindevices May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@memsharded looks good now

@memsharded memsharded merged commit 406d3f7 into conan-io:release/1.25 May 19, 2020
2 checks passed
@memsharded memsharded deleted the hotfix/deploy_symlinks branch May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants