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

add support for exports and exports_sources as methods #6945

Merged
merged 8 commits into from May 27, 2020

Conversation

solvingj
Copy link
Contributor

@solvingj solvingj commented May 1, 2020

Changelog: Feature: Add export() and export_sources() methods, that provide the self.copy() helper to add files to recipe or sources in the same way as the corresponding attributes.
Docs: conan-io/docs#1733


Notice.-

Description in the PR doesn't match actual implementation merged


Note: Waiting to add docs until this feature until implementation has been reviewed.

closes: #6943
close #7102

Also related to others: #6944

Goals:
The gap it intends to fulfill, and how it should/should-not interact with other methods should be clearly explained in future documentation. Here's a summary to start with.

The goal of exports_sources is to provide out-of-source recipes a method to cache sources in the same way that exports_sources does. This could theoretically replace all uses of source() method. Described another way, it represents a "caching" version of source() method.

Implementation
The exports_sources method overlaps two existing concepts:

  • source() method
  • exports_sources attribute

The current implementation deserves some explanation as we may decide to change it. The current implementation was the path of least resistance: make two modifications to the call path of exports_sources logic today:

  • Add a condition on the type of exports_sources member, and if its a function, execute it
  • Add the self.copy method with export_sources being the working directory

Everything else about the behavior would be identical to the exports_sources attribute. Most importantly, the timings at which it was executed are the same.

Alternative Ideas for implementation

  • different method names
    • current implementation makes attribute and method mutually exclusive
    • in contrast to requires/requirements
    • in contrast to source/exports_sources
    • might be ok
  • make self.copy() available in the source() method
    • substantially different internally, different timings, etc.
  • just teach source() to use exports_sources directory as the working directory
    • would need to make conditional for backward compatibility
    • timings/ hooks different from exports_sources, further refactoring may be necessary
  • create a new method source_exported(), identical to source, but exports_sources as working
    • drop-in-replacement for source, no further recipe changes required, it just works
    • inherently backward compatible
    • timings/hooks different from exports_sources, further refactoring may be necessary
    • name is descriptive and appropriate, but once-again still confusing

Notes for discussion...

  • Understand Interactions with

    • local flow
    • editable packages
    • scm
  • 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.

if callable(conanfile.exports_sources):
conanfile.deps_cpp_info = DepsCppInfo()
conanfile.copy = FileCopier([origin_folder], destination_source_folder)
conanfile.export_sources()

Choose a reason for hiding this comment

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

Suggested change
conanfile.export_sources()
conanfile.exports_sources()

@prince-chrismc
Copy link

prince-chrismc commented May 4, 2020

As for #6876

I tested a recipe from CCI, I added the following method

    def exports_sources(self):
        self.copy(pattern="patches/**")
        tools.get(**self.conan_data["sources"][self.version])
        extracted_dir = self.name + "-" + self.version
        os.rename(extracted_dir, self.copy._dst_folder)

Which worked exactly as intended, certainly makes it much easier to bundle the sources. I need to think about how I can use this as part of a final solution. But it's certainly a welcomed improvement.

class ExportsMethodTest(unittest.TestCase):

def setUp(self):
self.server = TestServer()
Copy link
Member

@memsharded memsharded May 5, 2020

Choose a reason for hiding this comment

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

You don't need servers for anything, simplify

"""


class ExportsMethodTest(unittest.TestCase):
Copy link
Member

@memsharded memsharded May 5, 2020

Choose a reason for hiding this comment

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

Do not put new tests in "integration" folder, that is to be removed.
Moved them to "functional/command/export" folder (create the folder, and move other export tests in "funcional/command" there too.

from conans.test.utils.tools import NO_SETTINGS_PACKAGE_ID, TestClient, TestServer

attribute_conanfile = """
from conans import ConanFile
Copy link
Member

@memsharded memsharded May 5, 2020

Choose a reason for hiding this comment

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

Use GenConanfile() generator if possible. Minor improvements to ``GenConanfile" can be done if necessary

if conanfile.exports:
tmp.extend(conanfile.exports) # conanfile.exports could be a tuple (immutable)
conanfile.exports = tmp
if os.path.exists(os.path.join(origin_folder, DATA_YML)):
Copy link
Member

@memsharded memsharded May 5, 2020

Choose a reason for hiding this comment

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

The data_yml needs to be exported always. Probably this should be done irrespective of the attribute/method approach

if callable(conanfile.exports_sources):
conanfile.deps_cpp_info = DepsCppInfo()
conanfile.copy = FileCopier([origin_folder], destination_source_folder)
conanfile.export_sources()
Copy link
Member

@memsharded memsharded May 5, 2020

Choose a reason for hiding this comment

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

PLease use:

conan_file.output.highlight("Running export_sources()")
with conanfile_exception_formatter(str(conan_file), "export_sources"):

to wrap it and provide better user messages in case of exceptions

@memsharded memsharded added this to the 1.26 milestone May 5, 2020
@memsharded memsharded requested a review from jgsogo May 5, 2020
@jgsogo
Copy link
Contributor

jgsogo commented May 5, 2020

Like the idea, and the possibilities it opens to record a snapshot of the sources for packages built with Conan Center, but I'd want to see how a recipe using it would look like: what's the difference between the exports, exports_sources and source method?

The example in #6945 (comment) is a bit asymmetric IMO, I see sources going from the recipe folder to the cache and sources arriving from outside to the recipe folder (and then to the cache)... which one is the working directory for this method?

I like the idea of running exports/exports_sources like methods but naming is confusing for me. Can you modify a recipe from Conan Center to use these methods?

Thanks!

@solvingj
Copy link
Contributor Author

solvingj commented May 5, 2020

@prince-chrismc

As for #6876

Which worked exactly as intended, certainly makes it much easier to bundle the sources. I need to think about how I can use this as part of a final solution. But it's certainly a welcomed improvement.

Indeed, there's a very important open question relating to one of the fundamental goals for "source packaging":

  • How to compose the "exported_sources" with per-packageid-sources generated in build()

This situation has existed for a long time, but we've never actually had to deal with it because we've never really entertained the idea of supporting any interaction with exports_sources folder and/or source folder except to copy them to build directory before build(). Some of the fundamental goals of "source packaging" are pushing us toward different interactions there, so we may have to create a strategy for this moving forward. However, that probably won't be done in this PR.

Probably refer to #6944 to follow that thread.

@solvingj
Copy link
Contributor Author

solvingj commented May 5, 2020

Like the idea, and the possibilities it opens to record a snapshot of the sources for packages built with Conan Center, but I'd want to see how a recipe using it would look like: what's the difference between the exports, exports_sources and source method?

The example in #6945 (comment) is a bit asymmetric IMO, I see sources going from the recipe folder to the cache and sources arriving from outside to the recipe folder (and then to the cache)... which one is the working directory for this method?

I like the idea of running exports/exports_sources like methods but naming is confusing for me. Can you modify a recipe from Conan Center to use these methods?

Thanks!

Good questions. I've modified the original PR comment to provide the goals and implementation details which hopefully answer your questions. Obviously, will need to be VERY clear in docs.

The most important addition is this... I already favor a different implementation from this one.

  • create a new method source_exported() (probably can find a better name)
    • identical to source, but uses exports_sources directory as working directory
    • this makes everything in the directory at the end of the method "automatically cached"
    • drop-in-replacement for source, no further recipe changes required, it just works
    • inherently backward compatible
    • timings/hooks different from exports_sources, further refactoring may be necessary
    • name is descriptive and appropriate, but once-again still confusing

Of note, we also created a function equivalent of exports() because it was easy and basically mirrored exports_sources in every way. There was no strong need or feature request. If we went with the implementation I just described here, I would just drop the exports() method because it would just be very different from this new method.

@memsharded
Copy link
Member

memsharded commented May 5, 2020

create a new method source_exported()

Yes, interesting approach too. Even if this was another feature in another PR, I think we need to have an overall strategy defined first. If the new method exports_sources() (with another naming?) will not be used in any case to download sources, I am fine with it, and the key might be to define the relevant folders adequately. I also think we need to proof check what we propose here that it works for the local flow too. Traditionally, the "export" and "source" methods have always been complicated things in the local flow.

@prince-chrismc
Copy link

prince-chrismc commented May 5, 2020

The example in #6945 (comment) is a bit asymmetric IMO, I see sources going from the recipe folder to the cache and sources arriving from outside to the recipe folder (and then to the cache)... which one is the working directory for this method?

The working directory was the recipe folder!

And yes, my goal is very far the intended usage of this feature.

You guys are doing some great work ❤️

@memsharded memsharded self-assigned this May 11, 2020
@memsharded
Copy link
Member

memsharded commented May 11, 2020

There is a problem with using exports_sources() method to do tools.download(). We do tons of conan export commands in ConanCenter to bootstrap things. This will take forever (now it is relatively fast).

@solvingj
Copy link
Contributor Author

solvingj commented May 11, 2020

For this reason, and others, I suggest that this feature should be re-implemented based on the current pipeline of the source() method rather than basing off the current exports_sources() attribute.

@memsharded
Copy link
Member

memsharded commented May 11, 2020

For this reason, and others, I suggest that this feature should be re-implemented based on the current pipeline of the source() method rather than basing off the current exports_sources() attribute.

Yes, but an implementation based on the source() flow will be way more difficult, most likely involving server-side changes, which typically take at least many months to implement and release. This is why I was looking for something based on the export flow.

@memsharded memsharded marked this pull request as ready for review May 22, 2020
jgsogo
jgsogo approved these changes May 25, 2020
Copy link
Contributor

@jgsogo jgsogo left a comment

Thinking about Conan v2.0 I wonder if we want to have both attribute and method available at the same time, or we should allow only one implementation (with the same name).

Besides that, please:

  • Update the PR description to match the implementation (existing comments can be moved to an issue if needed)
  • Waiting for docs

@jgsogo
Copy link
Contributor

jgsogo commented May 27, 2020

Something to add to docs: if function export() is a good place to store custom data to conandata.yml file, I'd like to see an example in the documentation (it will help here: #7050, probably enough to close that issue).

Copy link
Contributor

@jgsogo jgsogo left a comment

Just a couple of comments, and we need to update the PR description and docs

conans/client/cmd/export.py Show resolved Hide resolved
@memsharded memsharded merged commit 21bb2e5 into conan-io:develop May 27, 2020
1 check 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.

[question] export_sources wildcards [feature] Support exports() and exports_sources() as methods
4 participants