-
-
Notifications
You must be signed in to change notification settings - Fork 230
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
module for creation of toolset projects for libs #296
base: main
Are you sure you want to change the base?
Conversation
10ab1e5
to
e72eced
Compare
@grafikrobot can you comment on this PR? |
@grisumbras sorry it's taking a long time to look at this. I've had some personal things to deal with. Unfortunately it's going to take me even longer. As I now have to finish writing some wg21 wording on a paper in the next 10 days. |
Sorry it took so long for me to look at this. But finally had time, the whole weekend free time, for this. I have to say I'm really confused by the changes even after hours of reading the code and imagining how this is meant to be used. So instead of a bunch single comments to specific code parts I'm going to list some questions I have roughly from beginning to end:
I also have some UI design comments/questions:
From a practical perspective I don't see why we need these many changes to various parts just for vcpkg support. I don't see how this is going to be maintainable long term. In that it looks like there's a lot of disparate coupling in what you're doing. My current inclination is to drop this PR entirely and start a different approach from scratch. |
I will first answer design/UI questions.
This does integrate with PM support, to the extent it is possible. The core of this PR is
As I've mentioned, the module "disables" itself when it sees that a project that it tries to create already exists. I've experimented with Conan, and Conan-created project (using your Implementation questions:
My original intention was to add the functionality to
They allow
Can you be more specific? As far as I can see, use of env variables was copied from
I answered this previously, but to reiterate, I added vcpkg package directories to the list of directories where files are searched for. This is kind of the same as looking in
Library discovery in
Not at the moment. Do you think there's benefit in such support? I have a suspicion it would hinder the ability to disable
When experimenting with using
It does. But possibly, I didn't understand your question. Can you be more specific?
Updated: added |
e72eced
to
e5ce2f0
Compare
@grafikrobot can we return to discussing this PR? |
The PR
The functionality is thoroughly described in the documentation also added by the PR. The intention is to allow project
authors to request external dependencies, and the users to be able to conveniently describe how to find them.
vckpg support is partcularly relevant on Windows, where it quickly becomes one of the most popular package managers, if not the default one.
Together all changes should allow (in the best case scenario) something like the following to work:
Also, if the project for external dependency already exists at the time of invocation of
external.lib
orexternal.alias
, themodule does not try to create it again, and does not create a target either. This makes it trivially overridable by e.g. Conan generators.
Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.I couldn't figure out how to mock existence of pre-installed packages, so no tests.