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

Docs about 'profile:host' and 'profile:build' #1629

Merged
merged 18 commits into from Mar 31, 2020

Conversation

jgsogo
Copy link
Member

@jgsogo jgsogo commented Mar 25, 2020

Docs for conan-io/conan#5594

Closes #1586

devtools/create_installer_packages.rst Outdated Show resolved Hide resolved
devtools/create_installer_packages.rst Outdated Show resolved Hide resolved
devtools/create_installer_packages.rst Outdated Show resolved Hide resolved
devtools/create_installer_packages.rst Outdated Show resolved Hide resolved
devtools/create_installer_packages.rst Outdated Show resolved Hide resolved
devtools/create_installer_packages.rst Outdated Show resolved Hide resolved
@jgsogo jgsogo marked this pull request as ready for review Mar 30, 2020
czoido
czoido approved these changes Mar 30, 2020
@czoido czoido merged commit 40fb238 into conan-io:develop Mar 31, 2020
2 checks passed
@jgsogo jgsogo deleted the doc/5594-xbuild branch Mar 31, 2020
self.copy("license*", dst="", src=self.nasm_folder_name, keep_path=False, ignore_case=True)

def package_info(self):
self.output.info("Using %s version" % self.nasm_folder_name)
self.env_info.path.append(os.path.join(self.package_folder, self.nasm_folder_name))
self.env_info.PATH.append(os.path.join(self.package_folder, "bin"))
Copy link
Contributor

@ytimenkov ytimenkov May 29, 2020

Choose a reason for hiding this comment

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

@jgsogo it should be cpp_info.bindir, no?

Copy link
Contributor

@ytimenkov ytimenkov May 29, 2020

Choose a reason for hiding this comment

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

I mean Iirc all helpers should use run_environment or virtualrunenv, so having bindirs should be enough. Did this change?

Copy link
Member Author

@jgsogo jgsogo Jun 2, 2020

Choose a reason for hiding this comment

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

Depending on where you use this recipe. If you use it with the new model with the two profiles, you just need to set cpp_info.bindirs to the bin directory (it is added by default). But if you want to use this same recipe using only one profile in the command line, you need to add the bin folder to the PATH.

My intention here was to write an example that works with the two scenarios, even if this is talking about the two profiles... do you think it is misleading? I can imagine people copy/pasting an example and IMO it is better if it works for the two scenarios (if it requires almost no extra code)

Copy link
Contributor

@ytimenkov ytimenkov Jun 2, 2020

Choose a reason for hiding this comment

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

I mean Conan handled well ld_library_path on Linux, path on windows (for dynamic libraries) as well as macos wieredness with environment jamming in some cases.

And it felt that use of environment variables for path/dll path was discouraged or obsolete, things just worked (with run_environment though).

And I see no reason for 2 contexts should interfere with this.

Copy link
Member Author

@jgsogo jgsogo Jun 2, 2020

Choose a reason for hiding this comment

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

🤔 You are right, RunEnvironment is already adding bin directories to PATH, so it works with one profile (this is what run_envrionment=True does) and with two profiles (logic that propagates environment from build to host does it already).

So this line is useless.

Copy link
Member Author

@jgsogo jgsogo Jun 4, 2020

Choose a reason for hiding this comment

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

Needed for the virtualenv generator (using one profile)

Copy link
Contributor

@ytimenkov ytimenkov Jun 4, 2020

Choose a reason for hiding this comment

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

Yes, that's why I merged them into one (in our company): as soon as you want to run a tool you need runenv. And this is 100% times since we have compiler and python as Conan packages.

Copy link
Contributor

@ytimenkov ytimenkov Jun 4, 2020

Choose a reason for hiding this comment

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

I thinkrun_environment=True could be a default for Conan 2.0. and virtualenv should behave as if running from recipe (thus all virtual* merged).

I don't see why it is a harmful default. And we get defaults wrong: explicit, const, noexcept? 😂

Copy link
Member Author

@jgsogo jgsogo Jun 4, 2020

Choose a reason for hiding this comment

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

Yes, Conan 2.0 will reconsiders the defaults for the self.run(...) and the environment that is populated before entering recipe functions like build(). We are also aware that all virtualxxxenv generators might be merged into one.

It's been many years of 1.x and our commitment to stability, we really are looking forward for the next major version and break some of the conventions/defaults.

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.

None yet

5 participants