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

xcodedeps: provide package root dir #11818

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

the-nic
Copy link
Contributor

@the-nic the-nic commented Aug 9, 2022

Changelog: Feature: Defines the PACKAGE_ROOT_<package> variable in XcodeDeps generated files.
Docs: conan-io/docs#2717

If one wants to access other folders than the provided ones (like
resources), the provided variables dont contain enough information
to access these folders.

Provide a new variable, PACKAGE_ROOT_<package>_<component> pointing
to the root directory of the package allowing to use it within xcode.

We have to pass the package folder as another parameter, because the
transitive_cpp_info variable does not contain the rootpath
attribute and we also cant access the package_folder attribute from
self._conanfile.dependencies.package_folder directly because
the package name is already formatted via _format_name.

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@czoido czoido self-assigned this Aug 9, 2022
@the-nic the-nic force-pushed the xcodedeps-root-dir branch from cc6de3f to 26b70aa Compare August 9, 2022 08:24
@czoido
Copy link
Contributor

czoido commented Aug 10, 2022

Hi @the-nic,

Thanks for the contribution.

I think that maybe it would be better to use the resdirs information from the cppinfo so that if a package have a directory for resources it is available. And a variable something like RESOURCE_PATHS_libname_libname[...][...][...] = ...

Is there a case where you need to access directly the package folder that is not covered by the case that you add the resdir to the cppinfo and have access to it?

@the-nic
Copy link
Contributor Author

the-nic commented Aug 10, 2022

I think that maybe it would be better to use the resdirs information from the cppinfo so that if a package have a directory for resources it is available. And a variable something like RESOURCE_PATHS_libname_libname[...][...][...] = ...

Certainly, this would be a good addition, too.

Is there a case where you need to access directly the package folder that is not covered by the case that you add the resdir to the cppinfo and have access to it?

The problem is, these are often multiple paths and if I have to reference a specific file from the package, it its impossible to do without the single root path. Additionally, the cmake generator also provides a package root path, so they become a bit more on par.

@czoido czoido assigned memsharded and unassigned czoido Aug 22, 2022
Copy link
Contributor

@czoido czoido left a comment

Choose a reason for hiding this comment

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

Hi @the-nic,
I was thinking that probably it would be better to define this variable in the file that includes all components for the package instead here, and not with the component suffix but just as: PACKAGE_ROOT_<package> because that information will be the same for all the components. WDYT?

@the-nic
Copy link
Contributor Author

the-nic commented Aug 22, 2022

Hi @the-nic, I was thinking that probably it would be better to define this variable in the file that includes all components for the package instead here, and not with the component suffix but just as: PACKAGE_ROOT_<package> because that information will be the same for all the components. WDYT?

Good point, I'll update the PR

@the-nic
Copy link
Contributor Author

the-nic commented Aug 22, 2022

Hi @czoido I tried your suggestion right now and the problem is that the even though the information is the same for all components, its not the same for all configurations. And only the component files seem to be configuration-aware. Otherwise, we'd have to update the package file on each time a new configuration is added...

@czoido
Copy link
Contributor

czoido commented Aug 23, 2022

Hi @the-nic,
Sorry, you are right, I did not take that into account. Then maybe it's better to just leave them in the components files, but remove the _{{comp_name}} and let them be defined in multiple times (I suppose it won't be a problem, but I had to test that first).

If one wants to access other folders than the provided ones (like
resources), the provided variables dont contain enough information
to access these folders.

Provide a new variable, `PACKAGE_ROOT_<package>` pointing
to the root directory of the package allowing to use it within xcode.

We have to pass the package folder as another parameter, because the
`transitive_cpp_info` variable does not contain the `rootpath`
attribute and we also cant access the package_folder attribute from
`self._conanfile.dependencies.package_folder` directly because
the package name is already formatted via `_format_name`.
@the-nic the-nic force-pushed the xcodedeps-root-dir branch from 26b70aa to 4a0ce76 Compare August 23, 2022 06:27
@the-nic
Copy link
Contributor Author

the-nic commented Aug 23, 2022

@czoido I have removed the component name from the root dir variable.

@czoido
Copy link
Contributor

czoido commented Aug 23, 2022

Thanks a lot @the-nic,
I'll do some checks and then I think it's good to merge.
Maybe in the future if we add more variables at package level it would be worth to make a separate file that defines them all and that is included from the file that includes the components, but if the multiple definition is not a problem I think this is good enough for the moment.
Thanks!

@memsharded memsharded assigned czoido and unassigned memsharded Aug 24, 2022
@czoido czoido merged commit b5c0ee0 into conan-io:develop Aug 24, 2022
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.

3 participants