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

[VCPKG] Using [core] in Build-Depends does not work #11602

Closed
JackBoosY opened this issue May 27, 2020 · 25 comments
Closed

[VCPKG] Using [core] in Build-Depends does not work #11602

JackBoosY opened this issue May 27, 2020 · 25 comments
Assignees
Labels
category:question This issue is a question

Comments

@JackBoosY
Copy link
Contributor

Describe the bug
When port A has default feature and port B depends on A[core], the default feature of port A will still be activated.

Environment

  • OS: [e.g. Windows/Linux etc...] All
  • Compiler: revision

To Reproduce
Steps to reproduce the behavior:

  1. Modify ports/rabit/CONTROL, set Build-Depends: dmlc to Build-Depends: dmlc[core]
  2. ./vcpkg install rabit
  3. See error
    Repro code when

Expected behavior
Any build that relies on other ports[core] should only build the port itself without any features.

Failure logs
N/A

Additional context
N/A

@JackBoosY JackBoosY added the category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) label May 27, 2020
@JackBoosY
Copy link
Contributor Author

Maybe the same reason with #11519.

@Neumann-A
Copy link
Contributor

When port A has default feature and port B depends on A[core], the default feature of port A will still be activated.

That is intended behavior/design decession assume port A,B and port A having features X,Y,Z. (X,Y defaulted)

If B Build-Depends on A[core] it means that B requires A[core]. but the user might want to use A and B together with it's default. Instead of having the user type vcpkg install A B it is enough to say vcpkg install B to get A with its default. Since the user did not specify A vcpkg assumes a reasonable default set of features which is X,Y and whatever B requires (in this case only core).
If the user really only wants A[core] feature the user has to be explicit about it which means: vcpkg install A[core] B.
So vcpkg assumes the default is in general ok if the user did explicitly opt out of it by explicitly installing only the minimal set of features. IMHO this is a very reasonable choice because the other way around might create confusion why certain expected features are missing. A lot of ports have features which are normally enabled by default by the port itself (e.g. cmake options which are on by default) and vcpkg should honor those defaults.

@JackBoosY
Copy link
Contributor Author

@Neumann-A According to documentation, the behavior of the installation port should be consistent with the behavior of the dependency installation.

@Neumann-A
Copy link
Contributor

@JackBoosY That behavior is not documented.

When using vcpkg install , some features will be enabled by default. These default features can be avoided by referring to the packages as [core] and features can be added by supplying them on the same installation line.

lets assume:
vcpkg install B
currently vcpkg translates that implicitly to [ () mark the implicit call]

  1. vcpkg install (A[default,feat]) B with B depending on A[feat]
    You suggest to change that to
  2. vcpkg install (A[core,feat]) B

From a user experience I personally prefer 1. while I understand why 2. would also be desirable.
Maybe add a flag for a feature minimal install? (e.g. --minimal-features and also an env variable to control the behavior)

I like 1. because I don't need to care about explicitly listing all default features I would want for a dependent port. Furthermore the control files of most ports are probably wrong and should always list A[core] instead of A as a default.

Currently:
Build-Depends: A means requires all of the default features while
Build-Depends: A[feat] requires minimally A[core,feat]

Required new meaning according to accomplish 2. without updating all Build-Depends: fields with <port>[core]
Build-Depends: A require A[core] only (will build A[core,default] if not explicitly asked otherwise) while
Build-Depends: A[feat] minimally A[core,feat]

So the problem lies probably in the listing in the Control files.

@cenit
Copy link
Contributor

cenit commented May 27, 2020

That is intended behavior/design decession assume port A,B and port A having features X,Y,Z. (X,Y defaulted)

Not so sure about that.
I clearly remember discussions where it was explained that if you wrote dependency to port[core], default features were explicitly avoided. What you say is that writing a dependency to port and to port[core] is the same, what I remember is that it shouldn’t be (why also duplicating behavior and making it different between cli interface and control interface?)
So I agree with @JackBoosY, this is a vcpkg bug if confirmed

@cbezault
Copy link
Contributor

cbezault commented May 27, 2020

The way it has been described to me in the past is that if portx is specified as a dependency it effectively means that you explicitly depend on all of the default features of portx. However if you write porty[core] you explicitly depend on only the core feature. portx[some-feature] also means that you have an explicit dependency on the default features of portx + some-feature.

However, in addition to the above, a decision was made to add an implicit dependency on the default features of all dependencies, unless that dependency is already installed or if in the command line you specify that you really only want the core feature.

i.e. From a clean vcpkg clone, vcpkg install portz will install all of dependencies of portz with all of their default-features enabled.
Say portz depends on portx[core]. Then vcpkg install portz will still install all of portx's default-features. vcpkg install portz portx[core] will install portz and only portx's core feature.

vcpkg install portx[core], vcpkg install portz will also result in only portx's core feature getting installed.

@cbezault
Copy link
Contributor

So, to be clear, I don't consider the described behavior to be a bug, though I'm not a fan of it.

@cenit
Copy link
Contributor

cenit commented May 27, 2020

There is (let me say) incoherence between command line interface and “CONTROL” interface. It is really a source of confusion, and I would have been really sure I was right about the intentions, before your comment...

@cbezault
Copy link
Contributor

Per @ras0219-msft
The rules are:

  1. Expand all references appropriately (A -> A[defaults], B[core] -> B[core])
  2. Install everything directly referenced
  3. Every package that wasn't already installed and wasn't mentioned on the command line also gets default features

@Neumann-A
Copy link
Contributor

Neumann-A commented May 27, 2020

Short

`X` means `X[default]`
`X[core]` means `X[core]`
`X[feature]  `means `X[default,feature]` (Inconsistent with the above but [core] is special)
`X[core,feature]` means `X[core,feature]`

In build-depends of port Y

X               means Y requires X[default] (historic reason. Should probably mean X is installed instead)
X[core]         means Y requires X[core] (Meaning: X is installed)
X[feature]      means Y requires X[default,feature] (Inconsistent with the above but [core] is special)
X[core,feature] means Y requires X[core,feature]

In vcpkg install Y with X in build depends

X               -> vcpkg install X[default] Y
X[core]         -> vcpkg install X[default] Y
X[feature]      -> vcpkg install X[feature, default] Y
X[core,feature] -> vcpkg install X[core, feature] Y

In vcpkg install X[feat2] Y with X in build depends as follows

X               -> vcpkg install X[default,feat2] Y
X[core]         -> vcpkg install X[default, feat2] Y
X[feature]      -> vcpkg install X[feature, default, feat2] Y
X[core,feature] -> vcpkg install X[core, feature, feat2] Y

In vcpkg install X[core,feat2] Ywith X in build depends as follows

X               -> vcpkg install X[default, feat2] Y
X[core]         -> vcpkg install X[core, feat2] Y
X[feature]      -> vcpkg install X[feature, default, feat2] Y
X[core,feature] -> vcpkg install X[core, feature, feat2] Y

The issue raised here is saying that the following should be changed:
vcpkg install Y with X in build depends

X[core]         -> vcpkg install X[core] Y     <-- only core

Since the user only said vcpkg install Y I would say vcpkg is free to do either vcpkg install X[core] Y or vcpkg install X[default] Y. From a most users point of view the expected install of X is probably one with the default features and not a feature minimal install. The user did not explicitly specify X either because he or she did not know that Y requires X or he doesn't care how X is build. More experienced users can opt-out of this behavior by defining vcpkg install X[core] Y to get the feature minimal installation.
Furthermore making the change to vcpkg install X[core] Y would als need to make the change that vcpkg install X[feat] Y is only vcpkg install X[core,feat] Y and not vcpkg install X[default,feat] Y. This change is inconvenient since a lot of people probably want default+feat instead of core+feat

@JackBoosY
Copy link
Contributor Author

JackBoosY commented May 28, 2020

If:

  1. we have a port named A, and it has a default feature featA.
  2. we also have a port named B, and it relies on A[core].

Expect behavor is:

  1. when we run command vcpkg install A. A[core,featA] will be installed. Because A stands for A[default].
  2. when we run command vcpkkg install B, only A[core] should be installed instead of A[core, featA].

@cbezault
Copy link
Contributor

@JackBoosY the entire above conversation suggests what you are seeing is by design not a bug.

@JackBoosY JackBoosY removed the category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) label May 28, 2020
@JackBoosY
Copy link
Contributor Author

@cbezault But now, if I install B, A[core,featA] will be installed. So, using [core] or not in Build-Depends is the same.

@cenit
Copy link
Contributor

cenit commented May 28, 2020

So, using [core] or not in Build-Depends is the same.

Exactly. They say it’s a design choice. I say it’s a strange one, but ok

@cbezault
Copy link
Contributor

It's not the same. If you wrote vcpkg install B A[core] only A[core] would be installed. However if in the Build-Depends you omitted core from A there would be no way to get only A[core]. You would always get A[core, featA].

@cbezault
Copy link
Contributor

So for your original issue. If you write vcpkg install rabit dmlc[core] you should just get dmlc[core] (unless something else also has dmlc with all its default features as a dependency).

@cenit
Copy link
Contributor

cenit commented May 28, 2020

Yes I understood. The problem for me is just that you are not supposed to know dependencies of your dependencies. And I was expecting vcpkg to install the minimum required for me, especially if the portfile is taking extra care of saying that it does not require anything more than the core version of another library...

@PhoebeHui
Copy link
Contributor

PhoebeHui commented May 28, 2020

I think the behaviour should be consistent when we use port[core], either we only install port[core] or depend on port[core].

eg: the dmlc default feature openmp shouldn't be installed since we specified to depend on dmlc[core]. it should be same with installing dmlc[core] only.

Source: rabit
Version: 0.1-2
Homepage: https:/dmlc/rabit
Description: rabit is a light weight library that provides a fault tolerant interface of Allreduce and Broadcast. It is designed to support easy implementations of distributed machine learning programs, many of which fall naturally under the Allreduce abstraction.
Build-Depends: dmlc[core]
Supports: !uwp

PS F:\vcpkg\src> ./vcpkg install rabit
Your feedback is important to improve Vcpkg! Please take 3 minutes to complete our survey by running: vcpkg contact --survey
Computing installation plan...
The following packages will be built and installed:
  * dmlc[core,openmp]:x86-windows
    rabit[core]:x86-windows
Additional packages (*) will be modified to complete this operation.
Starting package 1/2: dmlc:x86-windows
Building package dmlc[core,openmp]:x86-windows...
-- Note: dmlc only supports static library linkage. Building static library.
...
PS F:\vcpkg\src> ./vcpkg install dmlc[core]
Computing installation plan...
The following packages will be built and installed:
    dmlc[core]:x86-windows
Starting package 1/1: dmlc:x86-windows
Building package dmlc[core]:x86-windows...
-- Note: dmlc only supports static library linkage. Building static library.
...

@JackBoosY
Copy link
Contributor Author

@cbezault @Neumann-A I still think that's a bug, because if you use the keyword core, the behavior of the installation or dependencies should be the same as only core.
And I don't think we should consider dependency, because it is declared in CONTROL, the user does not need to consider its dependencies.

@linquize
Copy link

linquize commented Aug 4, 2020

Another example is libssh
Default-Features: crypto
Feature: crypto Build-Depends: libssh[mbedtls]
Feature: openssl Build-Depends: openssl

vcpkg install libssh[openssl] installs openssl[core] and also mbedtls[core,pthreads] the latter is not desired.

@Neumann-A
Copy link
Contributor

@linquize because the correct syntax is vcpkg install libssh[core,openssl]. if you do not specify core, default features are assumed

@linquize
Copy link

linquize commented Aug 5, 2020

How can I completely skip openssl if I choose mbedtls?
Do we need to change the port?

@linquize because the correct syntax is vcpkg install libssh[core,openssl]. if you do not specify core, default features are assumed

@BillyONeal
Copy link
Member

@ras0219 indicates that this is intentional behavior to reduce the likelihood of needing to rebuild the world if someone adds dependencies incrementally; to make vcpkg work more like "apt" and friends. That doesn't preclude adding an option --minimal-dependencies or similar which would give this behavior.

Also note that @strega-nil 's "manifest mode" should probably imply this --minimal-dependencies setting should it be added.

@JackBoosY
Copy link
Contributor Author

JackBoosY commented May 27, 2021

here: https:/microsoft/vcpkg-tool/blob/d37abb3e83a3a639931886b1e6c6bb56d180e756/src/vcpkg/dependencies.cpp#L189-L209

                else if (request_type != RequestType::USER_REQUESTED)
                {
                    // If the user did not explicitly request this installation, we need to add all new default features
                    auto&& new_defaults = get_scfl_or_exit().source_control_file->core_paragraph->default_features;
                    std::set<std::string> defaults_set{new_defaults.begin(), new_defaults.end()};

                    // Install only features that were not previously available
                    if (auto p_inst = m_installed.get())
                    {
                        for (auto&& prev_default : p_inst->ipv.core->package.default_features)
                        {
                            defaults_set.erase(prev_default);
                        }
                    }

                    for (const std::string& feature : defaults_set)
                    {
                        // Instead of dealing with adding default features to each of our dependencies right
                        // away we just defer to the next pass of the loop.
                        out_reinstall_requirements.emplace_back(m_spec, feature);
                    }

Automaticly set all the default features. And for request_type, it always be RequestType::AUTO_SELECTED in https:/microsoft/vcpkg-tool/blob/d37abb3e83a3a639931886b1e6c6bb56d180e756/src/vcpkg/dependencies.cpp#L240:

            RequestType request_type = RequestType::AUTO_SELECTED;

@ras0219-msft
Copy link
Contributor

I'm closing this issue because all behaviors described in this thread are by design. Please see the discussion above about the rationale.

If anyone feels that there is a not-by-design behavior given the explanation above (especially [1]), please open a new issue.

[1] #11602 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:question This issue is a question
Projects
None yet
Development

No branches or pull requests

10 participants