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

Fix regression related to merge_directories with symlinked folders #5342

Merged
merged 15 commits into from Jun 13, 2019

Conversation

Projects
None yet
3 participants
@jgsogo
Copy link
Member

commented Jun 12, 2019

Changelog: Bugfix: Do not copy directories inside a symlinked one
Docs: omit

closes #5329

In #5237 we fixed an issue, but also introduced a wrong behavior that was failing for some use cases (see new test: folders inside symlinked folders with more than one level of indirection). The folders inside the symlinked one were being copied (so the symlink folder was generated) and the final function creating the symlinked folder failed because the folder to create was already there (it has already been created due to the previous recursion).

Just for quick reference, this is the layout of the source folder for the new test (SymlinkExportSources::test_create_source):

image

image

The intention of test (test_broken_in_local_sources) is just to prove that broken symlinks are detected before reaching the modified functions.

@jgsogo jgsogo added this to the 1.16.1 milestone Jun 12, 2019

@jgsogo jgsogo referenced this pull request Jun 12, 2019

Closed

exports_sources fails to properly handle symlinks #5329

3 of 3 tasks complete

@lasote lasote requested a review from memsharded Jun 12, 2019

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

I've added here the test proposed in #5345 to validate that broken symlinks are being handled before reaching the modified code.

memsharded added some commits Jun 13, 2019

Show resolved Hide resolved conans/client/source.py Outdated
@memsharded
Copy link
Contributor

left a comment

And after my changes, the build is green: https://conan-ci.jfrog.info/blue/organizations/jenkins/ConanTestSuite/detail/PR-5342/8/pipeline.

It is true that there is insuficent test coverage for all the possible cases of symlinks and exports, exports-sources, SCM, etc., so we should be careful with every change we do, because otherwise we will be breaking again with each change. Please @jgsogo

  • Review changes
  • Add tests to ensure these changes make sense.
@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

On top of this branch I've added some tests here: #5353. I didn't want to merge them into this PR before talking about some use cases.

SCM introduces a new set of problems, as you have pointed in your previous comments. I think that we need to implement the same behavior for the consumer workflow (conan install <ref> --build or conan export+conan install) and for the producer (conan create . <ref>), and if there is a conflict we should take into account the behavior without the local_sources_pah optimization, which is the one we get with export+install (in the tests, those are parameterized with use_optimization=False).

Ideally we should have also the same behavior for SVN and GIT, but unfortunately, they behave differently related to linked folders: Git doesn't accept linked folders while SVN does. Is this a problem we want to handle?

About abs paths, totally agree, but let's do it for the linked files too (done )

I'm also not sure if we want to fix all those tests for the v1.16.1 (it can delay the release to the next week) or to v1.17. wdyt @lasote?

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

Please, @memsharded, review again and let me know about it, main points:

  • same behavior for files and folders
  • linked files/folders outside the source directory are skipped (there is a placeholder for #5063)
  • links are created as relative paths

I find the implementation quite clear, with all the logic into the function link_to_rel.

Some tests that fail in #5353 still fail: all of them are related to SCM scenarios with local_sources_path optimization (not intended to be covered in this PR).

@jgsogo jgsogo requested a review from memsharded Jun 13, 2019

@memsharded
Copy link
Contributor

left a comment

yeah, looking good now, thanks!

@memsharded memsharded merged commit 2ad5ac5 into conan-io:release/1.16.1 Jun 13, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@jgsogo jgsogo deleted the jgsogo:regression/5329-symlinks branch Jun 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.