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

[poc] Add wrappers|shims around executables #7324

Closed
wants to merge 52 commits into from

Conversation

jgsogo
Copy link
Contributor

@jgsogo jgsogo commented Jul 7, 2020

Changelog: (Feature | Fix | Bugfix): Describe here your pull request
Docs: https://github.com/conan-io/docs/pull/XXXX

Note.- Highly experimental, just an idea, the implementation should follow another way.

  • Add testing
  • Make it configurable
  • Reduce duplicated code

In this POC I'm creating wrappers around the executables declared in the cpp_info object. This is intended mainly for build requirements, with this approach the environment from one build_requires won't pollute the environment from other build_requires (thinking that build-requires are isolated graphs and they don't conflict with each other), this is a problem not only for envvars that contain values and might collide, but to ensure that they will use the proper shared libraries. It also solves the issue with Macos and SIP, from within CMake it is possible to call protoc and the wrapper will take care of adding the values to DY/LD_LIBRARIES_PATH and shared libraries will be found.

Protoc scenario reproduced here: https://github.com/jgsogo/conan-xbuild-scenarios/tree/master/protobuf

To think about (maybe restrictions):

  • This should only be about "final" applications, if build_requires expand isolated graphs, each wrapper should activate the environment for that graph (yes, there might be collisions inside the same graph, but these ones are up to the user).

  • The graph is resolved in runtime with the inputs of the user, these environments need to be created at conan install <consumer> time, they cannot be created when the package itself is installed (unless we check the lockfile).

  • Is it legit to use env_info to propagate data to consumers? I mean, maybe the wrapper should activate all the environment, and no env_info is populated with data from the build_requires (only the PATH to these wrappers). If the user want to propagate data, they should use user_info field.

     class BuildRequire(ConanFile):
         name = "br"
    
         def package_info(self):
             self.cpp_info.executables = ["exe1", "exe2"]
             self.env_info.MYVAR = "value"
     class Consumer(ConanFile):
         def build_requirements(self):
             self.build_requires("br/version", context="ctxt1")
    
         def build(self):
             assert not "MYVAR" in os.environ
             with self.context("ctxt1"):
                 assert "MYVAR" in os.environ
                 assert tools.which("exe1")
    
             self.run("exe1", run_environment="ctxt1")

Other alternatives/approaches:


This PR (or similar) would:

@jgsogo
Copy link
Contributor Author

jgsogo commented Oct 15, 2020

In the context of this PR, there are a couple of related issues we need to talk about:

  • cpp_info.exe new attribute.

  • environment context for the build() method (two profiles approach): with the shims it is no longer needed to populate the environment with everything coming from the build-requires, only the PATH to the shims folder is needed. If we stop populating the environment automatically, we need to provide a way to explicitly activate it (I know there is an issue about it, but I cannot find it).

Copy link
Contributor

@lasote lasote left a comment

Choose a reason for hiding this comment

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

Is this somehow taking care of the shared libraries issue?

environment context for the build() method (two profiles approach): with the shims it is no longer needed to populate the environment with everything coming from the build-requires, only the PATH to the shims folder is needed. If we stop populating the environment automatically, we need to provide a way to explicitly activate it (I know there is an issue about it, but I cannot find it).

Not sure about that, what if the build require doesn't have any executable but indeed is populating some environment variables? (like configuration or whatever). Probably the only thing to change (as a good practice for shims) is to avoid modifying the env.PATH from the build require.

@jgsogo
Copy link
Contributor Author

jgsogo commented Oct 16, 2020

There is a problem if we populate automatically the environment, and it is demonstrated in the tests added to this PR: if two different build-requires (of the same recipe) populate the same variable (here the LIBRARY_ENVVAR) then the values will conflict and only one of them will be taken into account. In these tests, have a look at how each build-requires populates LIBRARY_ENVVAR with a different value and it is the value used (runner1-value and runner2-value):

image


In MacOS it will be the same for the DYLD_LIBRARY_PATH, if we do isolate the graphs from different build-requires, they could depend on different versions of the same library without conflicting. We cannot append everything to the environment, only the first dynamic library will be found and used (DLL Hell).


Together with shims (no need to populate the PATH or DYLD...) we can force the user to activate explicitly the environment, something like: you can activate the environment of a build-requires and run other commands using that environment.

image

  • Using shims fixes the issue for dynamically injected build-requires. These build-requires (this is speculative) are usually trying to inject a tool, an executable, from the recipe POV I wouldn't expect them to provide only an environment variable.
  • Build-requires that are hardcoded in the recipe, are more likely to provide environment variables, but, as they are hardcoded the recipe can activate the environment explicitly.

@lasote
Copy link
Contributor

lasote commented Oct 19, 2020

I understand and agree but still, we cannot stop populating the environment of the requirements always. Maybe only when they declare an exe but still weak. Probably we still lack a better model. Probably (cpp components-like) the correct model idea would be something similar to:

self.cpp_info.exe = "cmake"
self.cpp_info.exe["cmake"].env.foo = "bar" # Only activated with the shim
self.cpp_info.env.var = "bar2" # Activated globally (and can collide)

WDYT?

@jgsogo
Copy link
Contributor Author

jgsogo commented Oct 19, 2020

I can't say. I'd need real examples of dynamic build-requires that need to inject their environment variables into the recipes they apply to. At first, I'd say that if the recipe is coupled with the build-requires (it is hardcoded/listed in the recipe itself) it is ok to activate the environment explicitly, what examples do we have for the other type of build-requires?

@lasote
Copy link
Contributor

lasote commented Oct 19, 2020

Nothing comes to my mind right now that you will consider it like the best practice. But people are reading env vars from build requires for sure. Declaring toolchains maybe. Just saying that the model as you described is not complete because you never know when the user really wants to propagate the env or not.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Seems a good step, lets talk about the strategy, I am thinking that having it as a generator could have advantages.

import textwrap


class LocalWorkflowTestCase(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Unused?

conan_file.info.env_values.add(name, value, package_name)


def _classify_dependencies(node):
Copy link
Member

Choose a reason for hiding this comment

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

Probably this deserves a previous refactor in a previous PR.

# I need the shims for the executables in the build-requires (only two-profiles approach)
if self._cache.config.shims_enabled:
logger.info("SHIMS: Writing shims for build-requires")
_, _, build_deps = _classify_dependencies(node)
Copy link
Member

Choose a reason for hiding this comment

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

And if the refactor is done, this could be moved later, so classify_dependencies() is called just one? It would be a bit more natural.

""")


def _generate_shim(name, conanfile, settings_os, output_path):
Copy link
Member

Choose a reason for hiding this comment

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

Is this a new Generator like the toolchains? something you can parameterize and control too?

}
""")

lib_h = textwrap.dedent("""
Copy link
Member

Choose a reason for hiding this comment

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

maybe use gen_function_xxxx() if possible.

@CLAassistant
Copy link

CLAassistant commented May 18, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 3 committers have signed the CLA.

✅ memsharded
❌ lasote
❌ jgsogo
You have signed the CLA already but the status is still pending? Let us recheck it.

@memsharded memsharded closed this May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Overcome Windows system PATH length limitation
7 participants