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

[feat] Introduce exception raised in 'configure' that will propagate only if 'build' #6952

Closed
wants to merge 5 commits into from

Conversation

jgsogo
Copy link
Contributor

@jgsogo jgsogo commented May 4, 2020

Changelog: Feature: Add ConanIWontBuild exception: this exception is being raised from the configure() method but it will trigger a failure only if build() method is executed.
Docs: https://github.com/conan-io/docs/pull/XXXX

On top of refactor in #6951

Introduces a ConanIWontBuild exception (please, suggest a proper name) that the user can raise from the configure() method, but it will be propagated only if Conan invokes the build method.

This can be useful for scenarios with compatible_packages where one configuration won't build, but there is a compatible package that can be used (see test in this PR).

  • Find a better name for the exception

  • It's up to the user to raise this function after any other ConanInvalidConfiguration

  • Conan will fail with the same code error as ConanInvalidConfiguration, is it ok?

@jgsogo jgsogo added this to the 1.26 milestone May 4, 2020
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

I am wondering, and thinking that it would be good to see an effect in the package_id too:

  • configure raise ConanIWontBuild
  • package_id computes an "INVALID-ID" for this binary (still able to fall back to other compatible package_ids)
  • if build() tries to build an INVALID-ID, it raises.

@SSE4
Copy link
Contributor

SSE4 commented May 12, 2020

I am not sure, but if you need to throw an exception only of package is being build, why not just throw it from the build method then? this for me sounds more logical and simple.

@jgsogo
Copy link
Contributor Author

jgsogo commented May 22, 2020

I am not sure, but if you need to throw an exception only of package is being build, why not just throw it from the build method then? this for me sounds more logical and simple.

This has been one of the best answers so far and it has taken me more than a week to think about why yes/not to do this. 🙌

(Maybe this is a symptom of a bad workflow in CCI, but) we need to know if the recipe will build for a given settings without running the build() method. We are doing all these checks in a Linux node and then, for those packageIDs that doesn't raise a ConanInvalidConfiguration we run Mac/Windows/Linux nodes to build them.

So this kind of exception should be known by Conan without running the build and it cannot be raised and propagated from the configure() because we wouldn't have a chance to use a compatible_package...

These are more or less the constraints. The exception as it works in this PR is valid for a solo-developer, in my machine it is ok if it raises in the build() method, but for CCI we need something like the conan package_id command I introduced to you and maybe (or not) this exception.

@SSE4
Copy link
Contributor

SSE4 commented May 22, 2020

yes, for me it sounds definitely as CI or jenkins workflow design limitation, which you are trying to solve in the conan client. I'd probably better re-design workflow, if possible, so it just discards builds raising in the build method, instead of adding first class conan feature just to address that specific problem. but, if there is no option...

@jgsogo
Copy link
Contributor Author

jgsogo commented May 25, 2020

The question would be, (regarding CCI), can we run conan install --build=xxxx right away and forget about this weird exception? We would waste some resources launching the node, docker image,... just to get an exception, probably not very important.

For the cppstd POC, it wasn't needed either. The extra thing needed was that conan package_id command that was able to retrieve the package_id of every compatible package. But it has nothing to do with this exception.

I think we need to imagine a valid use-case or this will reach a dead-end

@memsharded
Copy link
Member

reviewed this. I think this is a new mode/behavior we need to build around the existing ConanInvalidConfiguration, for Conan 2.0:

  • It doesn't raise directly, but accumulates internally
  • It might have a PACKAGE_ID_INVALID new package_id mode. It can show in graphs, invalid
  • It is not retrieved from servers, it will not call the http apis
  • It will only raise if the package is really needed, like a full install
  • Can be the mode for build_requires that only exists in one platform and are used via --pr:build=mybuildprofile, but later in the host system is used without specifying that build profile, and only the host artifacts are necessary (running, deploying).

@memsharded memsharded modified the milestones: 1.26, 2.0 Jun 1, 2020
@jgsogo
Copy link
Contributor Author

jgsogo commented Jun 2, 2020

Not exactly related to this exception, but I think it has something to do with the strategy of falling back to available binaries when build is not required.

Talking about tools, we agree that some of them belong to a package that also contains libraries (protobuf) so in the general use-case we need all the settings to identify these packages. But, when we are consuming only the tool (in the build context), we don't care about some of those settings.

In some scenarios, we could think that providing a really short profile for the build context could be enough:

profile:build

[settings]
os=Linux

if I only want a binary that runs, I don't care about other settings, I'd prefer static, Release, x64,... but if another binary is available just use it instead of failing and force me to build. From the package_id perspective, it looks like the compatible packages feature is enough, but this profile will fail to assign settings because arch missing, compiler missing,... as they are listed in the settings row inside the recipe.

There are two paths: one for the generation of packages and one for the consumption.

@memsharded
Copy link
Member

I think this has been superseded by #8053

@memsharded memsharded closed this Nov 30, 2020
@jgsogo jgsogo deleted the feat/exception-build branch December 1, 2020 14:53
@jgsogo jgsogo removed this from the 2.0 milestone Dec 1, 2020
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