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

Auto origin deduced as none if no origin #7048

Conversation

braindevices
Copy link
Contributor

@braindevices braindevices commented May 18, 2020

Changelog: Fix: Do not modify scm attribute when the origin remote cannot be deduced.
Changelog: Bugfix: Do not allow uploading packages with missing information in the scm attribute.
Docs: omit

▶️ See #7095 for additional implementation on top of this PR.
▶️ Updated CLI: #7107

closes #7033

  • 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.

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.

Copy link
Member

@memsharded memsharded left a comment

This is looking good thank you!

Asking for another review, to check too about the minor details (might be subjective opinion, so I prefer to have someone else review).

Assigning this to 1.26, it is almost ready

if scm.is_local_repository():
output.warn("Repo origin looks like a local path: %s" % origin)
output.success("Repo origin deduced by 'auto': %s" % origin)
if origin is None:
output.warn("origin is None, upload' command will prevent uploading recipes with None values in these fields.")
Copy link
Member

@memsharded memsharded May 19, 2020

Choose a reason for hiding this comment

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

I think it would be better to keep scm_data.url = "auto" and not assign it None. It is more symmetric to the scm.revision behavior.

Copy link
Member

@memsharded memsharded May 19, 2020

Choose a reason for hiding this comment

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

And if necessary to change output messages, check for the "auto" string.

@@ -127,8 +127,9 @@ def test_auto_git(self):
conanfile = base_git.format(directory="None", url=_quoted("auto"), revision="auto")
self.client.save({"conanfile.py": conanfile, "myfile.txt": "My file is copied"})
create_local_git_repo(folder=self.client.current_folder)
self.client.run("export . user/channel", assert_error=True)
self.assertIn("Repo origin cannot be deduced", self.client.out)
self.client.run("export . user/channel", assert_error=False)
Copy link
Member

@memsharded memsharded May 19, 2020

Choose a reason for hiding this comment

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

Do not use assert_error=False, it is the default, can be omitted.

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

jgsogo commented May 19, 2020

As I commented on the issue, I would prefer to keep scm_data.url = "auto", I think that it will require fewer changes as it should already be handled. Is there any reason to differentiate both scenarios?

@braindevices
Copy link
Contributor Author

braindevices commented May 21, 2020

@jgsogo We did not prevent user to set url=None in scm, I saw people used scm={url=None} to avoid the 'auto', thus I thought this was by design. If we should not support 'scm_data.rul=None' we should check that and throw error on it.

Copy link
Member

@memsharded memsharded left a comment

Great job, thanks!

@@ -295,8 +295,8 @@ def _upload_recipe(self, ref, conanfile, retry, retry_wait, policy, remote, remo
if policy != UPLOAD_POLICY_FORCE:
# Check SCM data for auto fields
if hasattr(conanfile, "scm") and (
conanfile.scm.get("url") == "auto" or conanfile.scm.get("revision") == "auto"):
raise ConanException("The recipe has 'scm.url' or 'scm.revision' with 'auto' "
conanfile.scm.get("url") == None or conanfile.scm.get("url") == "auto" or conanfile.scm.get("revision") == "auto"):
Copy link
Member

@memsharded memsharded May 21, 2020

Choose a reason for hiding this comment

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

Having a second thought, if there are users that are doing url=None, and now we start to block them: will they be surprised? Why are they doing this? The problem is that such information is not reproducible for conan builds (but the revision is there, they might use it in CI? Maybe with conan inspect is possible to read the revision and the URL is hardcoded in the CI?

Let's make sure we are not breaking other users without a clear value/alternative or without declaring it a bug.
Asking for @jgsogo feedback.

Copy link
Contributor Author

@braindevices braindevices May 21, 2020

Choose a reason for hiding this comment

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

when the url=None, it is not possible to upload anyway.

Copy link
Contributor

@jgsogo jgsogo May 26, 2020

Choose a reason for hiding this comment

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

If we want to cover in this PR the use-case where the SCM won't work because url is None, then we should cover other scenarios too: IMHO, if type is None or revision is None we should probably fail to upload the recipe.

@jgsogo

This comment has been minimized.

Copy link
Contributor

@jgsogo jgsogo left a comment

Please, consider my previous comment before moving forward, I'm open to alternatives, maybe I'm missing something.

@memsharded
Copy link
Member

memsharded commented May 25, 2020

I would prefer to keep the exported conanfile.py without changes, like we do when there are uncommitted files.

I am not sure what you mean. It seems the proposed behavior is good:

  • If scm_data.url == "auto", then try to deduce origin
  • If origin is defined, assign it to scm_data.url
  • If origin is not defined, it will keep the "auto"

Can you please clarify what you mean?

@jgsogo

This comment has been minimized.

@jgsogo
Copy link
Contributor

jgsogo commented May 26, 2020

I need to revisit this PR taking into account that it tries to handle another use case too, when the URL is declared as None explicitly by the user scm.url=None. Let me review this one again.


if scm_data.url is None:
output.warn(
"origin is None, upload' command will prevent uploading recipes with None values in these fields.")
Copy link
Contributor

@jgsogo jgsogo May 26, 2020

Choose a reason for hiding this comment

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

I would just change the lines around the exception and return without substituting the revision (no modification, like we do when the repo is not pristine):

    if not origin:	
        output.warn("Repo origin cannot be deduced, 'auto' fields won't be replaced."
                    " 'conan upload' command will prevent uploading recipes with 'auto'"
                    " values in these fields.")
        local_src_path = scm.get_local_path_to_url(origin)
        return scm_data, local_src_path

No need to modify is_local_repository or get_remote_url functions.

@@ -295,8 +295,8 @@ def _upload_recipe(self, ref, conanfile, retry, retry_wait, policy, remote, remo
if policy != UPLOAD_POLICY_FORCE:
# Check SCM data for auto fields
if hasattr(conanfile, "scm") and (
conanfile.scm.get("url") == "auto" or conanfile.scm.get("revision") == "auto"):
raise ConanException("The recipe has 'scm.url' or 'scm.revision' with 'auto' "
conanfile.scm.get("url") == None or conanfile.scm.get("url") == "auto" or conanfile.scm.get("revision") == "auto"):
Copy link
Contributor

@jgsogo jgsogo May 26, 2020

Choose a reason for hiding this comment

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

If we want to cover in this PR the use-case where the SCM won't work because url is None, then we should cover other scenarios too: IMHO, if type is None or revision is None we should probably fail to upload the recipe.

@memsharded
Copy link
Member

memsharded commented May 26, 2020

As this PR seems fine, lets merge it and we can contribute the remaining checks ("type=None, revision=None") later.

@memsharded memsharded merged commit 8609870 into conan-io:develop May 26, 2020
2 checks passed
jgsogo added a commit to jgsogo/conan that referenced this pull request May 26, 2020
memsharded added a commit that referenced this pull request May 27, 2020
* new tests for SCM with None fields

* rollback some changes in #7048 that modifies public tools

Co-authored-by: memsharded <james@conan.io>
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] cannot export local repo when using scm with auto
3 participants