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

Use the final package folder as the 'conanfile.package_folder' attribute for the 'pre_package' hook #5600

Merged
merged 1 commit into from Aug 20, 2019

Conversation

jgsogo
Copy link
Contributor

@jgsogo jgsogo commented Aug 9, 2019

Changelog: Bugfix: Use the final package folder as the conanfile.package_folder attribute for the pre_package hook.
Docs: omit

This is a bugfix for the experimental hooks feature.

The package_folder assigned to the conanfile object (to use its value in the pre_package hook) uses a different value for the export-pkg workflow and for the create_package one. (The current value was introduced here https://github.com/conan-io/conan/pull/3555/files#diff-94888d4df1b14d7fdea15cb92ad304ecR17)

IMO, it makes sense to use always the package_folder there (the folder where the package will be stored).

@jgsogo jgsogo requested a review from danimtb August 9, 2019 13:20
@jgsogo jgsogo changed the title Same package folder assigned to 'conanfile' for 'export-pkg` and 'create' workflows Use the final package folder as the 'conanfile.package_folder' attribute for the 'pre_package' hook Aug 9, 2019
Copy link
Member

@danimtb danimtb left a comment

Choose a reason for hiding this comment

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

Is there an issue filled for this PR? Please link it in the description of the PR.

Am not opposed to the change, but the reason for this implementation was that the user might find useful to perform some task in the userspace package folder here as this is a "pre" hook (doing some check in that folder to be able to block the export package action).

Any other action/check to be done in the cache package folder could be done ina post_package hook.

Also notice that that folder is the same in the conan create flow as there is no different package folder to point to.


Important: This will require a change in the documentation

@jgsogo
Copy link
Contributor Author

jgsogo commented Aug 12, 2019

There is no issue for this PR, I found this (IMO) bug having a look at other issue related to the hooks. Also, this behavior is not documented.

I think that the behavior should match the one when using conan create and in that workflow the package_folder is pointing to the directory where the package will be stored (empty one at this moment), so I think that it should be the same for the export-pkg command: it should point to the target directory where the package will be created.

@danimtb , I just need to agree with you before pushing this forward 😄

Copy link
Member

@danimtb danimtb left a comment

Choose a reason for hiding this comment

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

I was asking for the issue just to be sure if anyone was asking for this change or if you found some issue with the folder being different. As I said, I have no objections to the change, just wanting to find the motivation behind it.

Also, I thought this was documented somewhere, but seems it is not.

@@ -15,7 +15,7 @@
def export_pkg(conanfile, package_id, src_package_folder, package_folder, hook_manager,
conanfile_path, ref):
mkdir(package_folder)
conanfile.package_folder = src_package_folder
Copy link
Contributor

Choose a reason for hiding this comment

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

We will break someone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take it for granted, but IMHO it is a bug: right now the package_folder for the pre-package-hook doesn't have the same meaning for the local commands (source folder) and for the create one (target folder).

If you consider it is not a bug but a feature, then feel free to close this PR and I think we should document this behavior, at least I find it quite unexpected.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it sounds weird. Could we add a new parameter to the pre_package with the src_package_folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...but in the conan create ... flow there is no such thing as a single src_package_folder

Copy link
Contributor

Choose a reason for hiding this comment

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

It can be None in that case. Is just a matter of documenting the parameter. My concern is, if we "fix" this, users won't be able anymore to access the origin folder and that is breaking.

Anyway, let's accept this. I don't really think someone is using a hook for the export-pkg right now.

@lasote lasote merged commit 7b3a122 into conan-io:develop Aug 20, 2019
@lasote lasote added this to the 1.19 milestone Aug 20, 2019
@jgsogo jgsogo deleted the fix/packager-hook-folder branch August 20, 2019 07:31
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

4 participants