conan-io / docs

conan.io reStructuredText documentation
http://docs.conan.io
MIT License
104 stars 352 forks source link

[question] `CONAN_V2_MODE` and `settings.os` access in `source()` method #2458

Open MarcelRaad opened 2 years ago

MarcelRaad commented 2 years ago

I'm using Conan 1.46.2 with CONAN_V2_MODE enabled. I have some recipes which have to download different source archives for different operating systems, exactly what https://docs.conan.io/en/1.46/reference/conanfile/methods.html#source descibes as "Maybe the only setting that makes sense" using there. Unfortunately, I'm getting

ConanV2Exception: Conan v2 incompatible: 'self.settings' access in source() method is deprecated.

This even completely breaks packaging pre-compiled libraries as described in that documentation link.

Is there any recommended workaround? Or has that just not been noticed yet?

memsharded commented 2 years ago

I think there is an inconsistency in the docs there. Please note that the example reads:

if platform.system() == "Windows":

And not using self.settings at all.

The core concept is there in the docs (read the blue note):

So in short: If the source code is the same, just the zipping is different, you might use platform.system() in source(). Otherwise, it is recommended to get the sources in the build() method.

MarcelRaad commented 2 years ago

Thanks, makes sense! I missed that the example uses platform.system(). Right, the only thing I'm interested in is what platform the sources are unpacked on.

For the use case of packaging pre-compiled binaries, this means downloading binaries for all platforms in source() and then using package() to only package the one for the target system, right?

memsharded commented 2 years ago

For the use case of packaging pre-compiled binaries, this means downloading binaries for all platforms in source() and then using package() to only package the one for the target system, right?

No, I think this would be cleaner, and more efficient to do just the right download in build(), and let package() be simpler. The idea is that the process of "creating" or "obtaining" the binaries is in the build() method. So if you are downloading from somewhere pre-compiled binaries, I'd recommend using build(). Unless you prefer to decouple that, retrieve your binaries independently of Conan, and then use conan export-pkg. But to automate with Conan, yes, please try build().

MarcelRaad commented 2 years ago

OK, sounds good. Then maybe the following sentences from the documentation should be adapted to that or mention that they only apply when distributing binaries for a single platform:

You can even create packages for pre-compiled libraries you already have, even if you don’t have the source code. You can download the binaries, skip the build() method and define your package() and package_info() accordingly.

Personally, CONAN_V2_MODE works for me then and this question can be closed. Thank you very much!

memsharded commented 2 years ago

Great. I will move this to the docs to improve that sentence (though new Conan 2.0 might happen earlier than it). Thanks!

spectras commented 1 year ago

Since this issue was not closed, I take the opportunity for this comment:

Otherwise, it is recommended to get the sources in the build() method.

The recipe tools patch and apply_conandata_patches from conan.tools.files are currently hardcoded to patch in the source_folder.

So if one has different sources depending on the platform, and needs to patch them, they are out of luck.

memsharded commented 1 year ago

But @spectras the apply_conandata_patches() already assumes a structure in the conandata.yml of the patches that is platform independent, so it should be able to be applied in the source() method. In other words, apply_conandata_patches() always applies the same patches, so it can be applied in the source() safely, or otherwise, a custom solution with conditionals is necessary (and that is more difficult to define a common structure, because patches can vary per OS, per compiler, etc)

spectras commented 1 year ago

That is true, it was more a nice to have, to be able to customize the behavior a bit and avoid writing a custom patching logic for simple cases. But this does raise the question of when it stops being a simple case.
Forget it, we had the case (as we are moving download of proprietary things from source() to build(), preparing for conan2), and we thought it could be a nice enhancement. But creating complex configuration on conan side is probably not worth the effort indeed. Thanks for the fast answer!