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: handle corner cases related to SCM with local sources optimization #4249

Merged
merged 50 commits into from Jan 29, 2019

Conversation

Projects
None yet
4 participants
@jgsogo
Copy link
Member

commented Jan 8, 2019

Changelog: Fix: Handle corner cases related to SCM with local sources optimization
Docs: Omit

It should fix some tests in PR #4193

Note to reviewers, these are the steps to follow:

  1. Check new tests
  2. Check modified tests
  3. Review implementation
  4. I think that some tests are duplicated (between the old and the new ones), I would remove the dupe ones in the old test suite because I think that the new ones are clearer and better structured. Which tests should we remove? (read this comment: #4249 (comment))

@tags: svn

@ghost ghost assigned jgsogo Jan 8, 2019

@ghost ghost added the stage: review label Jan 8, 2019

@memsharded
Copy link
Contributor

left a comment

Please check broken CI

@@ -20,8 +20,10 @@ class ScmtestConan(ConanFile):
"""
client = TestClient()
client.save({"conanfile.py": conanfile})
client.runner("git init .", cwd=client.current_folder)

This comment has been minimized.

Copy link
@memsharded

memsharded Jan 21, 2019

Contributor

Why? The test case was failing even if it wasn't a git repo. Maybe leave the test as it was, and if anything, adding a new one with a git repo too.

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jan 21, 2019

Author Member

Without creating the Git repository this test fails (because it cannot get the URL to the remote). It is related to the comment about the difficult to understand block. If I execute conditionally that block for Git, then this test (and many others) can be kept as before.

This comment has been minimized.

Copy link
@memsharded

memsharded Jan 28, 2019

Contributor

Got it. Maybe an error test, if it is not a git repo, what is the error message. Who is launching it and at which step? Hint: should probably be on export time, not on "create" time.

This comment has been minimized.

Copy link
@memsharded

memsharded Jan 28, 2019

Contributor

What if:

  • URL is defined hardcoded with full url, not "auto"
  • revision=auto
Show resolved Hide resolved conans/model/scm.py Outdated
Show resolved Hide resolved conans/client/cmd/export.py Outdated
Show resolved Hide resolved conans/test/functional/scm/scm_test.py
Show resolved Hide resolved conans/test/unittests/client/cmd/source/test_run_scm.py Outdated
Show resolved Hide resolved conans/client/source.py Outdated

@ghost ghost assigned jgsogo Jan 21, 2019

@ghost ghost assigned jgsogo Jan 28, 2019

# Generate the scm_folder.txt file pointing to the src_path
src_path = scm.get_repo_root()
save(scm_src_file, src_path.replace("\\", "/"))
captured = scm_data.capture_origin or scm_data.capture_revision

if scm_data.url == "auto":

This comment has been minimized.

Copy link
@memsharded

memsharded Jan 28, 2019

Contributor

Check with a test of export+upload, without having a remote repo origin, and it should fail.

@@ -20,8 +20,10 @@ class ScmtestConan(ConanFile):
"""
client = TestClient()
client.save({"conanfile.py": conanfile})
client.runner("git init .", cwd=client.current_folder)

This comment has been minimized.

Copy link
@memsharded

memsharded Jan 28, 2019

Contributor

Got it. Maybe an error test, if it is not a git repo, what is the error message. Who is launching it and at which step? Hint: should probably be on export time, not on "create" time.

@@ -20,8 +20,10 @@ class ScmtestConan(ConanFile):
"""
client = TestClient()
client.save({"conanfile.py": conanfile})
client.runner("git init .", cwd=client.current_folder)

This comment has been minimized.

Copy link
@memsharded

memsharded Jan 28, 2019

Contributor

What if:

  • URL is defined hardcoded with full url, not "auto"
  • revision=auto
Show resolved Hide resolved conans/test/functional/scm/scm_test.py
@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Jan 28, 2019

I've added some tests and modified one behavior after talking with @memsharded; please @lasote have a look at them and let us know your opinion:

  • auto keywords, both for url and for revision, can only we used if the repository has a remote, otherwise it will return an ERROR with: ERROR: Repo origin cannot be deduced. This happens as early as possible, in the export command.

    We think this is expected for url=auto, but we are not sure if this is expected for revision=auto too (having a hardcoded url). We could keep going, as the url is already known and use the local sha for the revision...

    This change implies adding a remote to some existing tests with scm.

  • When there is no repo (the working copy is not a repo), the command export will fail if there is an auto for the url or for the revision.

jgsogo added some commits Jan 29, 2019

@lasote lasote merged commit a11b8a6 into conan-io:develop Jan 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 Jan 29, 2019

@jgsogo jgsogo deleted the jgsogo:issue/4222-scm-monorepo-auto branch Jan 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.