Repository: conan-io/conan
Pull request: #3812 Remove system requirements from cache
Related Issue: #3812
Changelog: Feature: Remove system requirements conan folders (not installed binaries) from cache

Docs: https://github.com/conan-io/docs/pull/1038

@PYVERS: Macos@py27, Windows@py36, Linux@py27, py34

closes #3812

- [X] Refer to the issue that supports this Pull Request.
- [X] If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
- [X] I've read the [Contributing guide](https://github.com/conan-io/conan/blob/develop/.github/CONTRIBUTING.md).
- [X] I've followed the PEP8 style guides for Python code.
- [X] I've opened another PR in the Conan docs repo to the ``develop`` branch, documenting this one. 

<sup>**Note:** By default this PR will skip the slower tests and will use a limited set of python versions. Check [here](https://github.com/conan-io/conan/blob/develop/.github/PR_INCREASE_TESTING.md) how to increase the testing level by writing some tags in the current PR body text.</sup>


Problem statement:
system_requirements files not shared between host/docker
I have an application depends on a library libA. libA's recipe has a system_requirements() method which installs a system library. libA's artifact is stored remotely. When I built the applications, it failed to find the system library installed by the libA's recipe.

Is system_requirements() transitive? If so, how to specify in the conanfile.txt or conanfile.py of application?
By the way, I don't want to build libA from source.

Hints from issue discussion:
In fact, this is caused by the same reason in https://github.com/conan-io/conan/issues/2262

Everytime libA is installed the system requirements method will be called if it hasn't installed before so I don't understand exactly what is the issue. What do you mean with `When I built the application`? The app is built in a different computer, right?  Is it not called when doing the `conan install`?
Actually I run the build inside Docker container and the local cache is stored on a persistent volume. So rerun the build of the application will not install system package anymore. 
It is a same problem as issue #2262. When I created this issue, I misunderstood the way how system_requirements() works.
I think it is better to store system_reqs.txt alongside with systems, so local cache can be freely shared among different nodes.

If system_reqs.txt are stored in the user’s home directory instead of CONAN_USER_HOME, then problem will be perfectly resolved.
I wouldn't recommend sharing the cache with the containers, to be honest. It can be problematic with the permissions of the storage and for example with the system requirements. I would say it more solid to communicate with a conan server (or better Artifactory community edition)
> I wouldn't recommend sharing the cache with the containers, to be honest. It can be problematic with the permissions of the storage and for example with the system requirements. I would say it more solid to communicate with a conan server (or better Artifactory community edition)

I don't think permission is a big issue, because Docker can use `-u` to set the user/group. 
The problem with conan server (or Artifactory) is that they can be slow to access. If the local cache can be shared locally, that would be a big plus.
By the way, all these `system_reqs.txt` are important for deploy the applications to target system. It would be nice to export all the `system_reqs.txt` in `deploy()` method.
Could a `conan remove --system_reqs` help?  #2262 @bilke @yangcha 
> Could a `conan remove --system_reqs` help? #2262 @bilke @yangcha

That will be great.

Commit message: #3812 --system-reqs can raise error

- Conan remove --system-reqs forwards the exception message
  when an error occurs.
- Re-order remove command arguments
- Add test to check system-reqs error message

Signed-off-by: Uilian Ries <uilianries@gmail.com>

Patch to review:
diff --git a/conans/client/cache.py b/conans/client/cache.py
index b09e462bd84..4616dc692d1 100644
--- a/conans/client/cache.py
+++ b/conans/client/cache.py
@@ -13,7 +13,7 @@
 from conans.model.profile import Profile
 from conans.model.ref import ConanFileReference
 from conans.model.settings import Settings
-from conans.paths import PUT_HEADERS
+from conans.paths import PUT_HEADERS, SYSTEM_REQS_FOLDER
 from conans.paths.simple_paths import SimplePaths
 from conans.unicode import get_cwd
 from conans.util.files import list_folder_subdirs, load, normalize, save
@@ -257,6 +257,11 @@ def delete_empty_dirs(self, deleted_refs):
                         break  # not empty
                 ref_path = os.path.dirname(ref_path)
 
+    def remove_package_system_reqs(self, reference):
+        conan_folder = self.conan(reference)
+        system_reqs_folder = os.path.join(conan_folder, SYSTEM_REQS_FOLDER)
+        shutil.rmtree(system_reqs_folder)
+
     def remove_locks(self):
         folders = list_folder_subdirs(self._store_folder, 4)
         for folder in folders:
diff --git a/conans/client/command.py b/conans/client/command.py
index 0f6f05184c2..2f06c458405 100644
--- a/conans/client/command.py
+++ b/conans/client/command.py
@@ -819,6 +819,8 @@ def remove(self, *args):
                                   "specifying the package ID"))
         parser.add_argument('-f', '--force', default=False, action='store_true',
                             help='Remove without requesting a confirmation')
+        parser.add_argument("-l", "--locks", default=False, action="store_true",
+                            help="Remove locks")
         parser.add_argument("-o", "--outdated", default=False, action="store_true",
                             help="Remove only outdated from recipe packages. " \
                                  "This flag can only be used with a reference")
@@ -829,8 +831,8 @@ def remove(self, *args):
                             help='Will remove from the specified remote')
         parser.add_argument('-s', '--src', default=False, action="store_true",
                             help='Remove source folders')
-        parser.add_argument("-l", "--locks", default=False, action="store_true",
-                            help="Remove locks")
+        parser.add_argument('-t', '--system-reqs', default=False, action="store_true",
+                            help='Remove system_reqs folders')
         args = parser.parse_args(*args)
 
         self._warn_python2()
@@ -854,6 +856,16 @@ def remove(self, *args):
             self._cache.remove_locks()
             self._user_io.out.info("Cache locks removed")
             return
+        elif args.system_reqs:
+            if not ref:
+                raise ConanException("Please specify a valid package reference to be cleaned")
+            try:
+                self._cache.remove_package_system_reqs(ref)
+            except Exception as error:
+                raise ConanException("Could not remove system_reqs: %s" % error)
+
+            self._user_io.out.info("Cache system_reqs from %s has been removed" % str(ref))
+            return
         else:
             if not args.pattern_or_reference:
                 raise ConanException('Please specify a pattern to be removed ("*" for all)')
diff --git a/conans/conan.py b/conans/conan.py
old mode 100644
new mode 100755
diff --git a/conans/test/integration/system_reqs_test.py b/conans/test/integration/system_reqs_test.py
index 28b1fc74743..52cbe10f345 100644
--- a/conans/test/integration/system_reqs_test.py
+++ b/conans/test/integration/system_reqs_test.py
@@ -1,6 +1,8 @@
 import os
 import unittest
 
+from conans.paths import SYSTEM_REQS_FOLDER
+
 from conans.model.ref import ConanFileReference, PackageReference
 from conans.test.utils.tools import NO_SETTINGS_PACKAGE_ID, TestClient
 from conans.util.files import load
@@ -22,7 +24,6 @@ def system_requirements(self):
 
 
 class SystemReqsTest(unittest.TestCase):
-
     def force_system_reqs_rerun_test(self):
         client = TestClient()
         files = {'conanfile.py': base_conanfile.replace("%GLOBAL%", "")}
@@ -92,8 +93,10 @@ def per_package_test(self):
 
     def global_test(self):
         client = TestClient()
-        files = {'conanfile.py': base_conanfile.replace("%GLOBAL%",
-                                                        "self.global_system_requirements=True")}
+        files = {
+            'conanfile.py': base_conanfile.replace("%GLOBAL%",
+                                                   "self.global_system_requirements=True")
+        }
         client.save(files)
         client.run("export . user/testing")
         client.run("install Test/0.1@user/testing --build missing")
@@ -128,8 +131,10 @@ def global_test(self):
 
     def wrong_output_test(self):
         client = TestClient()
-        files = {'conanfile.py':
-                 base_conanfile.replace("%GLOBAL%", "").replace('"Installed my stuff"', 'None')}
+        files = {
+            'conanfile.py':
+            base_conanfile.replace("%GLOBAL%", "").replace('"Installed my stuff"', 'None')
+        }
         client.save(files)
         client.run("export . user/testing")
         client.run("install Test/0.1@user/testing --build missing")
@@ -139,3 +144,45 @@ def wrong_output_test(self):
         pref = PackageReference(ref, "f0ba3ca2c218df4a877080ba99b65834b9413798")
         load_file = load(client.cache.system_reqs_package(pref))
         self.assertEqual('', load_file)
+
+    def remove_system_reqs_test(self):
+        ref = ConanFileReference.loads("Test/0.1@user/channel")
+        client = TestClient()
+        files = {'conanfile.py': base_conanfile.replace("%GLOBAL%", "")}
+        client.save(files)
+        system_reqs_path = os.path.join(
+            client.cache.package_layout(ref).conan(), SYSTEM_REQS_FOLDER)
+
+        # create package to populate system_reqs folder
+        self.assertFalse(os.path.exists(system_reqs_path))
+        client.run("create . user/channel")
+        self.assertIn("*+Running system requirements+*", client.user_io.out)
+        self.assertTrue(os.path.exists(system_reqs_path))
+
+        # a new build must not remove or re-run
+        client.run("create . user/channel")
+        self.assertNotIn("*+Running system requirements+*", client.user_io.out)
+        self.assertTrue(os.path.exists(system_reqs_path))
+
+        # error must not remove anything
+        with self.assertRaisesRegexp(
+                Exception, "ERROR: Please specify a valid package reference to be cleaned"):
+            client.run("remove --system-reqs")
+        self.assertTrue(os.path.exists(system_reqs_path))
+
+        # error must show exception message
+        with self.assertRaises(Exception) as error:
+            client.run("remove --system-reqs foo/bar@foo/bar")
+            self.assertIn("ERROR: Could not remove system_reqs: [Errno 2] No such file or directory:", error.exception)
+        self.assertTrue(os.path.exists(system_reqs_path))
+
+        # remove system_reqs global
+        client.run("remove --system-reqs Test/0.1@user/channel")
+        self.assertIn("Cache system_reqs from Test/0.1@user/channel has been removed",
+                      client.user_io.out)
+        self.assertFalse(os.path.exists(system_reqs_path))
+
+        # re-create system_reqs folder
+        client.run("create . user/channel")
+        self.assertIn("*+Running system requirements+*", client.user_io.out)
+        self.assertTrue(os.path.exists(system_reqs_path))


Code review comments:
diff_hunk: @@ -139,3 +144,45 @@ def wrong_output_test(self):
         pref = PackageReference(ref, "f0ba3ca2c218df4a877080ba99b65834b9413798")
         load_file = load(client.cache.system_reqs_package(pref))
         self.assertEqual('', load_file)
+
+    def remove_system_reqs_test(self):
+        ref = ConanFileReference.loads("Test/0.1@user/channel")
+        client = TestClient()
+        files = {'conanfile.py': base_conanfile.replace("%GLOBAL%", "")}
+        client.save(files)
+        system_reqs_path = os.path.join(
+            client.cache.package_layout(ref).conan(), SYSTEM_REQS_FOLDER)
+
+        # create package to populate system_reqs folder
+        self.assertFalse(os.path.exists(system_reqs_path))
+        client.run("create . user/channel")
+        self.assertIn("*+Running system requirements+*", client.user_io.out)
+        self.assertTrue(os.path.exists(system_reqs_path))
+
+        # a new build must not remove or re-run
+        client.run("create . user/channel")
+        self.assertNotIn("*+Running system requirements+*", client.user_io.out)
+        self.assertTrue(os.path.exists(system_reqs_path))
+
+        # error must not remove anything
+        with self.assertRaisesRegexp(
+                Exception, "ERROR: Please specify a valid package reference to be cleaned"):
+            client.run("remove --system-reqs")
+        self.assertTrue(os.path.exists(system_reqs_path))
+
+        # error must show exception message
+        with self.assertRaises(Exception) as error:
+            client.run("remove --system-reqs foo/bar@foo/bar")
+            self.assertIn("ERROR: Could not remove system_reqs: [Errno 2] No such file or directory:", error.exception)
path: conans/test/integration/system_reqs_test.py
text: @user1:
I'm not sure the user is expecting this to happen. Probably only if something weird happens like permissions or whatever. If the recipe doesn't exist it should fail with that message. Otherwise if the recipe exists but there is no cached system requirements it should exit normally.

@author:
so, should we ignore errors, as made to `--locks` ?

@author:
Done, errors will be ignored.

@user2:
why ignore? permission errors happen, especially on Windows (e.g. due to the AV software, search indexers, etc.). as a user, I would prefer commands to report if they weren't success, and return correct exit code in that case.
but we should clearly distinguish cases of missing recipe vs permission issues for sure. 

@author:
If we change this behavior for system-reqs, I recommend do the same for `--locks`

@user1:
As @user2 said I don't want to ignore the failures. But if a recipe doesn't exist it should fail by saying "the recipe doesn't exist", not a weird error. And if there is no system requirements folder for a recipe that exists, it should end with ok. (I think that test is missing)

@author:
understood

@author:
@user2 @user1  done! now it will raises friendly error messages.