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 copying symlink to directories (2) #7655

Merged
merged 8 commits into from Oct 20, 2020

Conversation

hifabian
Copy link
Contributor

@hifabian hifabian commented Sep 3, 2020

Changelog: Bugfix: Copy symbolic links to directory with deploy generator.
Docs: conan-io/docs#1830

(corrected user compared to #7654 to accept CLA)

fixes #7647
(no updated tests)

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@danimtb
Copy link
Member

danimtb commented Sep 10, 2020

Thank you for your contribution. I will reach you with some tests regarding the changes here.

@danimtb
Copy link
Member

danimtb commented Oct 5, 2020

Hi @hifabian and sorry for the delay.

I have added a test for the fix you proposed.
Please consider adding this commit danimtb@9b5978d to this PR and merge develop branch again so we can move this forward.

Thank you!

@danimtb danimtb added this to the 1.31 milestone Oct 5, 2020
@danimtb danimtb mentioned this pull request Oct 5, 2020
5 tasks
@danimtb
Copy link
Member

danimtb commented Oct 13, 2020

ping @hifabian

@hifabian
Copy link
Contributor Author

Thank you for writing the tests!
Your commit should be added now and merged with the current develop branch.

if self._package_folders:
lines.extend([' tools.mkdir(os.path.join(self.package_folder, "{}"))'
''.format(folder) for folder in self._package_folders])
if self._package_folders_link:
Copy link
Member

Choose a reason for hiding this comment

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

@danimtb

I would say that this is an overly test-specific thing, that this should be implemented directly in the test, leaving GenConanfile() more generic. Only common conanfile patterns should be here.

It is true that the _package_files_link was already there, but that pattern is used just exactly once in the whole codebase, so probably that deserved to be specified in the test directly, and not generalized here.

Asking @jgsogo for a second opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think the same. GenConanfile must be something generic... even if it cannot be used for some tests. Sometimes it is also better to write the plain conanfile source if it helps to understand what we are testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

...but this argument also applies to the with_package_file(..., link) that is already in the GenConanfile, only used once.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is what I meant with:

It is true that the _package_files_link was already there, but that pattern is used just exactly once in the whole codebase, so probably that deserved to be specified in the test directly, and not generalized here.

package_files_link shouldn't be in GenConanfile, but in the specific test that is using it.

Copy link
Member

Choose a reason for hiding this comment

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

done!

@memsharded memsharded requested a review from jgsogo Oct 19, 2020
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Please @danimtb move the test specific code to the test, and leave GenConanfile generic

@danimtb danimtb requested a review from memsharded Oct 19, 2020
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

👍

Only a couple of details related to the tests.

conans/test/functional/generators/deploy_test.py Outdated Show resolved Hide resolved
class DeployGeneratorSymbolicLinkFolderTest(unittest.TestCase):

def setUp(self):
conanfile = textwrap.dedent("""
Copy link
Contributor

Choose a reason for hiding this comment

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

We typically indent the content inside the textwrap.dedent

Copy link
Member

Choose a reason for hiding this comment

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

done!

danimtb and others added 2 commits Oct 20, 2020
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
jgsogo
jgsogo approved these changes Oct 20, 2020
@jgsogo jgsogo merged commit 9af2121 into conan-io:develop Oct 20, 2020
2 checks passed
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.

[bug] Deploy generator does not copy symbolic links to directories
4 participants