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

Feature/b2 rooted dep projects #4211

Merged
merged 3 commits into from Jan 14, 2019

Conversation

Projects
None yet
3 participants
@grisumbras
Copy link
Contributor

commented Dec 29, 2018

Changelog: Fix: Allow referring to projects created by b2 generator for dependencies with absolute paths.
Docs: Omit

  • Refer to the issue that supports this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.

@grisumbras grisumbras force-pushed the grisumbras:feature/b2-rooted-dep-projects branch from f7a2b3c to 3aed999 Dec 29, 2018

@grisumbras

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2018

@grafikrobot, do you have any opinion on whether this should be merged, or I should go some other direction with the fix, or maybe it's the docs that should be changed?

@jgsogo jgsogo referenced this pull request Jan 2, 2019

Closed

b2 generator does not define projects as rooted #4200

3 of 3 tasks complete
@lasote

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

@grisumbras is there any created issue for the current PR? Could you create it?
About the PR itself, I would love @grafikrobot to take a look.

@grafikrobot

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

Sorry... I was on vacation. First, this is not a bug fix. When I wrote the generator I intentionally did not make the targets global to allow multiple conan config locations. Which is something useful for large projects that have could have multiple libraries and execs using different versions of packages and still allow them to be isolated. Second, you can get the same effect by having the conan info at the root of your project, AFAIK. If that's not the case, then it would indeed be a bug. But this would not be the fix for the bug as this prevents the use case of localized package usage.

@grisumbras

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

Yes, it fixes #4200. But I indeed want @grafikrobot to share his opinion, because there might be some implications I don't see, or maybe this was the intended behaviour.

@grafikrobot

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

Looking at what I wrote in the docs:

For a root level project those could be referenced with an absolute project path, for example /clara.

I would suspect that if that's not working, it's a bug in b2 itself :-( And in the meantime best solution might be to change the docs to mention how to do the use-project if you want the rooted paths.

@@ -385,6 +385,7 @@ def b2_toolset_version(self):
conanbuildinfo_project_template = '''\
# {name}
project-define {name} ;
use-project /{name} : {name} ;

This comment has been minimized.

Copy link
@grafikrobot

grafikrobot Jan 3, 2019

Contributor

I think the better fix is something like this (note, not tested, just eyeballing it):

if [ project.is-jamroot-module [ project-define {name} ] ]
{
    use-project /{name} : {name} ;
}

Although putting an equivalent of that in the project-define rule in the prefix would be better.

@lasote lasote removed the type: bug label Jan 4, 2019

@grisumbras grisumbras force-pushed the grisumbras:feature/b2-rooted-dep-projects branch from 3aed999 to 417a150 Jan 10, 2019

@grisumbras

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

Rebased and updated to follow the suggestions. @grafikrobot, can you look at it?

@lasote

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

Thanks @grafikrobot and @grisumbras for your help!

@lasote lasote added this to the 1.12 milestone Jan 14, 2019

@lasote lasote merged commit 33f15dd into conan-io:develop Jan 14, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.