Repository: conan-io/conan
Pull request: core.scm:local_url=allow|block
Changelog: Feature: Introduce ``core.scm:local_url=allow|block`` to control local folder URLs in conandata ``scm``.
Changelog: Fix: Raise an error when trying to upload a package with a local folder URL in ``conandata`` ``scm``. 
Docs: https://github.com/conan-io/docs/pull/3801

Close https://github.com/conan-io/conan/issues/16591




Problem statement:
[bug] Recipes with invalid remote URLs in conandata can be uploaded
### Describe the bug

Conan 1.64.1

### How to reproduce it

When using the `export()`/`source()` methods in combination with manually implemented url and commit capturing (as recommended in the docs as replacement for the deprecated `scm` attribute) there is one code path which can result in Conan uploading invalid recipes to a remote. Let's suppose we have the following export method:

```python
def export(self):
    git = Git(self, self.recipe_folder)
    scm_url, scm_commit = git.get_url_and_commit()
    update_conandata(self, {"sources": {"commit": scm_commit, "url": scm_url}})
```

The `get_url_and_commit()` method will fail as expected when there are uncommitted changes on the local working copy. It will warn when the local commit is not in the remote (but still return a valid url), which is already problematic. But the actual critical code path is when either the local working copy has no remote set (which can happen in some Gitlab CI merge request pipelines where a separate working copy is created that contains only a single detached ref) or the remote is temporarily unreachable. In both of these cases Conan will silently set the url to the path of the working copy in the local filesystem. This path gets written into the conandata.yml and Conan will happily upload that to the remotes afterwards.

There are several reasons why I think this is a bug:
- Conanfile implementors must manually handle this in the `export()` methods, causing duplicate code in all recipe or alternatively a mandatory python_requires with the shared code
- It is hard to determine the error state because the method encodes this state into the url return value instead of adding a separate boolean that indicates the error, so we'd have to parse the url and decide if it's a valid url or a filesystem path
- Intercepting this code path globally is not possible without having to mock-patch the `get_url_and_commit()` method from a `pre_export()` hook (see below) because the conandata format is not standardized, so we can not deterministically parse it in a `post_export()` hook or the pipelines since we don't know how the url gets stored by the recipe
- The old `scm` attribute prevented upload of recipes with invalid urls, so I consider this a regression

This was breaking our release builds due to temporary Git remote connection issues, leading to broken releases in the remotes. Here is the hook that I'm currently using to mitigate the problem on our side, but I'd rather not put that into production because it could break any day:

```python
from urllib.parse import urlparse
from unittest.mock import patch
from conan.errors import ConanException
from conan.tools.scm import Git


original_get_url_and_commit = Git.get_url_and_commit

def custom_get_url_and_commit(self):
    url, commit = original_get_url_and_commit(self)
    try:
        url_parsed = urlparse(url)
        if url_parsed.scheme not in ["http", "https", "ssh"]:
            raise ValueError()
        return url, commit
    except ValueError:
        raise ConanException("Invalid URL returned by get_url_and_commit(). This might be caused by a missing remote on the working copy or temporary Git remote connection issues.")

def pre_export(output, conanfile, conanfile_path, reference, *args, **kwargs):
    global patcher
    patcher = patch.object(Git, 'get_url_and_commit', new=custom_get_url_and_commit)
    global mock_method
    mock_method = patcher.start()
```

I need some advice on how to proceed here without having to mock-patch or creating our own Conan fork.

Hints from issue discussion:
Hi @jasal82 

Thanks for your report.

I am having a look to the code, and investigating how this could be improved.

In the meantime, I think the hook approach is not bad, but I would handle this differently, to make it more solid. Instead of patching the method in a ``pre-export`` hook, I would do a ``post-export`` hooks that reads the ``conandata.yml`` and checks the recorded url and raise accordingly.
I think the central question is why a file system path instead of an url is considered a valid return value for `get_url_and_commit()`. What is the use-case here? Is it to support building a recipe on a local source tree which is not a working copy? IMHO getting a path instead of an url is surprising and unexpected, so I'd rather have the entire use-case handled differently. For example by not running export/source code at all if we're in a completely detached source tree environment. However, due to the fact that the conanfile-in-source-repo case is now handled the exact same way as the conanfile-needs-additional-sources-from-somewhere-else case this would be very difficult to implement.
Alternatively there could be a command line flag or conf which makes the build fail if that warning at the end of `get_url_and_commit()` is generated.
The main reason for this behavior is to allow the local developer flow without needing to push commits to the server. In order to have both the ``scm`` feature running and being able to test and locally create packages without pushing commits to the server, which is a big blocker for practically 100% of users, it is necessary that the ``scm`` feature also works seamlessly locally, using the local commits and not the server ones.

I agree that we want to improve the robustness to avoid unexpectedly uploading to the server local paths. I am checking 2 possibilities atm:
- Checking in the ``upload`` process, reading the ``scm`` (should be normalized in Conan 2) and raising accordingly. This might be done by default, and provide an opt-out
- Failing early in the ``Git`` methods, based on some configuration. At the moment it doesn't seem possible to change the current default, it would be too breaking, but at least having a conf that helps preventing this, as an opt-in.
- Maybe even both? 

> Alternatively there could be a command line flag or conf which makes the build fail if that warning at the end of get_url_and_commit() is generated.

This already exists in Conan 2, that has ``warnings-as-errors`` configuration. We could even add a new ``warning tag`` to fail on this specific warning.
I understand the local workflow case, but currently two different root causes are merged into one error state:
1. Local commit not in remote -> this is the local workflow case, you could still return a valid url because the working copy has a remote set
2. Working copy has no remote or remote not reachable -> this is different and should not result in the same warning and same error state

Commit message: core.scm:local_url=allow|block

Patch to review:
diff --git a/conan/tools/scm/git.py b/conan/tools/scm/git.py
index 1c8a12fc06d..117a064b8dd 100644
--- a/conan/tools/scm/git.py
+++ b/conan/tools/scm/git.py
@@ -34,6 +34,7 @@ def __init__(self, conanfile, folder=".", excluded=None):
                 self._excluded = c.get("core.scm:excluded", check_type=list)
             else:
                 self._excluded = conf_excluded
+        self._local_url = global_conf.get("core.scm:local_url", choices=["allow", "block"])
 
     def run(self, cmd, hidden_output=None):
         """
@@ -176,16 +177,14 @@ def get_url_and_commit(self, remote="origin", repository=False):
         in_remote = self.commit_in_remote(commit, remote=remote)
         if in_remote:
             return url, commit
-        # TODO: Once we know how to pass [conf] to export, enable this
-        # conf_name = "tools.scm:local"
-        # allow_local = self._conanfile.conf[conf_name]
-        # if not allow_local:
-        #    raise ConanException("Current commit {} doesn't exist in remote {}\n"
-        #                         "use '-c {}=1' to allow it".format(commit, remote, conf_name))
-
-        self._conanfile.output.warning("Current commit {} doesn't exist in remote {}\n"
-                                       "This revision will not be buildable in other "
-                                       "computer".format(commit, remote))
+        if self._local_url == "block":
+            raise ConanException(f"Current commit {commit} doesn't exist in remote {remote}\n"
+                                 "Failing according to 'core.scm:local_url=block' conf")
+
+        if self._local_url != "allow":
+            self._conanfile.output.warning("Current commit {} doesn't exist in remote {}\n"
+                                           "This revision will not be buildable in other "
+                                           "computer".format(commit, remote))
         return self.get_repo_root(), commit
 
     def get_repo_root(self):
diff --git a/conans/client/cmd/uploader.py b/conans/client/cmd/uploader.py
index cff1385a99c..a21d59823f6 100644
--- a/conans/client/cmd/uploader.py
+++ b/conans/client/cmd/uploader.py
@@ -7,7 +7,7 @@
 from conans.client.source import retrieve_exports_sources
 from conans.errors import ConanException, NotFoundException
 from conan.internal.paths import (CONAN_MANIFEST, CONANFILE, EXPORT_SOURCES_TGZ_NAME,
-                          EXPORT_TGZ_NAME, PACKAGE_TGZ_NAME, CONANINFO)
+                                  EXPORT_TGZ_NAME, PACKAGE_TGZ_NAME, CONANINFO)
 from conans.util.files import (clean_dirty, is_dirty, gather_files,
                                gzopen_without_timestamps, set_dirty_context_manager, mkdir,
                                human_size)
@@ -81,10 +81,17 @@ def __init__(self, app: ConanApp, global_conf):
         self._global_conf = global_conf
 
     def prepare(self, upload_bundle, enabled_remotes):
+        local_url = self._global_conf.get("core.scm:local_url", choices=["allow", "block"])
         for ref, bundle in upload_bundle.refs().items():
             layout = self._app.cache.recipe_layout(ref)
             conanfile_path = layout.conanfile()
             conanfile = self._app.loader.load_basic(conanfile_path)
+            url = conanfile.conan_data.get("scm", {}).get("url") if conanfile.conan_data else None
+            if local_url != "allow" and url is not None:
+                if not any(url.startswith(v) for v in ("ssh", "git", "http", "file")):
+                    raise ConanException(f"Package {ref} contains conandata scm url={url}\n"
+                                         "This isn't a remote URL, the build wontt be reproducible\n"
+                                         "Failing because conf 'core.scm:local_url!=allow'")
 
             if bundle.get("upload"):
                 self._prepare_recipe(ref, bundle, conanfile, enabled_remotes)
diff --git a/conans/model/conf.py b/conans/model/conf.py
index 1a36d24a5f9..745c6d81930 100644
--- a/conans/model/conf.py
+++ b/conans/model/conf.py
@@ -52,6 +52,7 @@
     "core.gzip:compresslevel": "The Gzip compression level for Conan artifacts (default=9)",
     # Excluded from revision_mode = "scm" dirty and Git().is_dirty() checks
     "core.scm:excluded": "List of excluded patterns for builtin git dirty checks",
+    "core.scm:local_url": "By default allows to store local folders as remote url, but not upload them. Use 'allow' for allowing upload and 'block' to completely forbid it",
     # Tools
     "tools.android:ndk_path": "Argument for the CMAKE_ANDROID_NDK",
     "tools.android:cmake_legacy_toolchain": "Define to explicitly pass ANDROID_USE_LEGACY_TOOLCHAIN_FILE in CMake toolchain",
diff --git a/test/functional/tools/scm/test_git.py b/test/functional/tools/scm/test_git.py
index af733605437..c566270d8a9 100644
--- a/test/functional/tools/scm/test_git.py
+++ b/test/functional/tools/scm/test_git.py
@@ -617,7 +617,7 @@ def test_full_scm(self, conanfile_scm):
                                                    "CMakeLists.txt": "mycmake"}, folder=folder)
 
         c = TestClient(default_server_user=True)
-        c.run_command('git clone "{}" .'.format(url))
+        c.run_command('git clone "file://{}" .'.format(url))
         c.run("create .")
         assert "pkg/0.1: MYCMAKE: mycmake" in c.out
         assert "pkg/0.1: MYFILE: myheader!" in c.out
@@ -648,7 +648,7 @@ def test_branch_flow(self, conanfile_scm):
         conanfile = self.conanfile_scm if conanfile_scm else self.conanfile_full
         url = git_create_bare_repo()
         c = TestClient(default_server_user=True)
-        c.run_command('git clone "{}" .'.format(url))
+        c.run_command('git clone "file://{}" .'.format(url))
         c.save({"conanfile.py": conanfile,
                 "src/myfile.h": "myheader!",
                 "CMakeLists.txt": "mycmake"})


Code review comments:
diff_hunk: @@ -81,10 +81,17 @@ def __init__(self, app: ConanApp, global_conf):
         self._global_conf = global_conf
 
     def prepare(self, upload_bundle, enabled_remotes):
+        local_url = self._global_conf.get("core.scm:local_url", choices=["allow", "block"])
         for ref, bundle in upload_bundle.refs().items():
             layout = self._app.cache.recipe_layout(ref)
             conanfile_path = layout.conanfile()
             conanfile = self._app.loader.load_basic(conanfile_path)
+            url = conanfile.conan_data.get("scm", {}).get("url") if conanfile.conan_data else None
+            if local_url != "allow" and url is not None:
+                if not any(url.startswith(v) for v in ("ssh", "git", "http", "file")):
+                    raise ConanException(f"Package {ref} contains conandata scm url={url}\n"
+                                         "This isn't a remote URL, the build wontt be reproducible\n"
path: conans/client/cmd/uploader.py
text: @user1:
```suggestion
                                         "This isn't a remote URL, the build won't be reproducible\n"
```