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

Always remove package folder in conan create #4918

Merged
merged 6 commits into from Apr 29, 2019

Conversation

Projects
None yet
3 participants
@danimtb
Copy link
Member

commented Apr 5, 2019

Changelog: Bugfix: Remove package folder in conan create even when using --keep-build
Docs: omit

  • Refer to the issue that supports this Pull Request: closes #4804
  • 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.

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 danimtb added the type: bug label Apr 5, 2019

@danimtb danimtb added this to the 1.15 milestone Apr 5, 2019

@ghost ghost assigned danimtb Apr 5, 2019

@ghost ghost added the stage: review label Apr 5, 2019

@danimtb danimtb removed their assignment Apr 5, 2019

@jgsogo
Copy link
Member

left a comment

If the idea is to delete the package_folder every time we are going to build, I would move the remove_folder_raising(package_folder) line to the place where the BUILD & PACKAGE comment is (here).

It would be out of the if skip_build and executed before the build() method, I'm thinking about a class with the cmake.build(), cmake.install() inside the build() method, we need to delete the package_folder before building (not after).

@danimtb

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

Yes, you are totally right! Did not think about that. Thanks

@danimtb

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

Requested changes done. I haven't added a test as the case fo cmake.install() in the build() method should be well covered in this test https://github.com/conan-io/conan/blob/develop/conans/test/functional/build_helpers/cmake_install_package_test.py#L46

@jgsogo

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

But that test has a client.run("remove * --force")... maybe another one could cover this use case (I don't know how to look for it).

@danimtb

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

That test should be enough to prove that the package folder is available in build() time. I have been thinking about the case of cmake.install() in build() together with conan create --keep-build and it wouldn't work either, as the build() is totally skipped when using --keep-build. That is one of the reasons we recomend using it in the package(): https://docs.conan.io/en/latest/howtos/cmake_install.html (we should update and include the reasons there).

@ghost ghost assigned danimtb Apr 12, 2019

@danimtb danimtb removed their assignment Apr 12, 2019

@danimtb danimtb requested a review from jgsogo Apr 12, 2019

@jgsogo
Copy link
Member

left a comment

We are almost there, but I have a couple of doubts 💭

Show resolved Hide resolved conans/client/installer.py Outdated
Show resolved Hide resolved conans/client/installer.py
Show resolved Hide resolved conans/client/installer.py Outdated
Show resolved Hide resolved conans/test/functional/command/create_test.py

@ghost ghost assigned danimtb Apr 12, 2019

@jgsogo

jgsogo approved these changes Apr 12, 2019

Copy link
Member

left a comment

👍

@danimtb danimtb removed their assignment Apr 14, 2019

pass
"""
test_conanfile = textwrap.dedent("""
from conans import ConanFile

This comment has been minimized.

Copy link
@memsharded

memsharded Apr 29, 2019

Contributor

If using dedent, better really indent the contents one more indent, so it is not aligned with test_conanfile.

@memsharded memsharded merged commit 829c29e into conan-io:develop Apr 29, 2019

2 checks passed

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

@ghost ghost removed the stage: review label Apr 29, 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.