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

[question] How to target a test package in a profile #16638

Closed
1 task done
spectras opened this issue Jul 9, 2024 · 16 comments · Fixed by #16639
Closed
1 task done

[question] How to target a test package in a profile #16638

spectras opened this issue Jul 9, 2024 · 16 comments · Fixed by #16639
Assignees

Comments

@spectras
Copy link

spectras commented Jul 9, 2024

What is your question?

Hello,

In my profile, I need to change flags for a specific package for compatibility reasons.
I do it like this:

[conf]
mypackage/*:tools.build:cxxflags+=["/w24146", "/w24308", "/w14703", "/w24996"]

So far so good. The package builds correctly.
The test fails though because, using the same profile, it will not get the additional cxxflags as mypackage/* is not applied to the test package afaik.

Is there a way to have those flags apply when running the test package too?

Have you read the CONTRIBUTING guide?

  • I've read the CONTRIBUTING guide
@memsharded memsharded self-assigned this Jul 9, 2024
@memsharded
Copy link
Member

Hi @spectras

Thanks for your question.
The right syntax to refer to a "consumer" conanfile, specially when it doesn't have a name (consumer conanfile.py without name, a conanfile.txt or a test_package/conanfile.py) is to use the & placeholder, check https://docs.conan.io/2/reference/config_files/profiles.html#profile-patterns

Please try:

&:tools.build:cxxflags+=["/w24146", "/w24308", "/w14703", "/w24996"]

to apply the flags both to the mypackage and to the test_package, as for the create case, the packaged conanfile.py is also considered to be the consumer or root of the graph at the time of creating the package (and test_package/conanfile.py will be the root of the graph when testing the package).

@memsharded
Copy link
Member

This test checks the behavior: #16639

@spectras
Copy link
Author

spectras commented Jul 9, 2024

Thanks for the fast answer. Unfortunately, while this works fine for just one package, it does not scale to a full workflow. Here is a bit more context:

  • We have several projects sharing a profile.
  • The packages in question are not any of those, they are a few dependencies.

That is, it's not just one, the profile has something like:

[conf]
tools.build:cxxflags=["/sdl"]
package_a/*:tools.build:cxxflags+=["/w24146"]
package_b/*:tools.build:cxxflags+=["/w24146", "/w24308"]
package_c/*:tools.build:cxxflags+=["/w14703", "/w24996"]

We build packages on our CI, one job per package doing conan create then uploading them to artifactory.

  • The issue of the & approach is we need a different profile for each package. This seems to defeat the point of having a profile.
  • For now we work around the issue by disabling testing (-tf="") for problematic packages. I was looking for a better way.

(If curious, the /sdl flags enables a set of compile-time and runtime checks — we then disable some of the compile-time checks for some packages known to break)

@memsharded
Copy link
Member

Why not disabling sdl altogether for the test_package?

@spectras
Copy link
Author

spectras commented Jul 9, 2024

I simplified a bit, there are more that just cxxflags, and there is more than just /sdl in it.

The job currently simply does conan create . --version $VERSION --user $USER --channel $CHANNEL -pr=production-win64 -pr:b=tools-win64

How would I go with overriding the cxxflags in the job?

The only way I see would require duplicating some of the profile's configuration inside each job (either on the command line or by writing a job-specific profile file).

At this point, this would mean:

  • the profile no longer reflects what is built, one would need to go and inspect the script for each package.
  • developers on their laptop cannot simply do a conan create, they need to go check the additional arguments the job for that package adds on the command line.
  • if someone changes the baseline flags in the profile, they must remember to go and also update each job.

Unless I am missing a way to do that that does not imply making the job aware of the content of the profile?

@memsharded
Copy link
Member

I have been trying a few things, but this seems not possible. The problem is that if you are defining per-package different confs in the profile, because even if we invented in a profile a place holder like test_package:tools.build:cxxflags, it would still not be enough, it would require something like a test_package-for-depname/*:tools.build:cxxflags, which honestly seems a bit too much to be a feature.

Note that this is a good thing that this is not possible. If you are creating a package, lets say dep1 with those /wXXXX, if consumers of the package will fail if they did not define such compiler flag themselves then there is something wrong. Either the profile should not disable those things per-package, but also to all possible consumers of that package. But as consumers of a package can't be known, it is not possible to define that in a profile.

In general, consumers of a package should receive their necessary build information either from profiles or from the package_info() of their dependencies, to make consumption of dependencies automatic and robust, but in seems in this case they are getting neither. If flags are "viral" and they are propagated to consumers of a package, setting them in profiles with something as package_b/*:tools.build:cxxflags+=["/w24146", "/w24308"] looks like a bit fragile approach.

Maybe the original issue is using a configuration for dependencies like tools.build:cxxflags while also building and creating those packages. If you are building them, maybe just adding the flags directly in the recipes? The tools.build:cxxflags is intended to inject flags to dependencies when those are just dependencies (non modifiable), but if you are creating the packages yourself, it sounds that you are using it more like a meta-build system than as a way to manage dependencies. If recipes define directly those compilation flags and then append to the self.cpp_info.cxxflags in package_info, then everything will be both self contained and robust for consumption.

@spectras
Copy link
Author

spectras commented Jul 9, 2024

That's right, in the end there are 2 separate issues:

  • The test package itself needs the /wXXXX. That's a test_package issue, it should be fixed in the recipe.
  • All consumers of the package need the /wXXXX be propagated.
    This seems to be about fixing the recipe, relying on conan mechanisms to carry that information.

⇒ I agree, handling that on the recipe side would be ideal.
Now that you mention it, it is indeed something I tried.

  • Initially I modified the broken recipes to pass relevant /wXXXX flags when is_msvc(self).
  • I then realized that the /sdl in tools.build.cxxflags always takes precedence over flags defined in the recipe.
  • This is what led me to trying to specify the /wXXXX in the profile.

This might be an XY problem. If there is a way to pass the /wXXXX flags from the recipe without the profile-level /sdl overriding them, that would make my initial question moot.

(I believe that means a way for the recipe to specify that some extra_cxxflags must end up after the tools.build.cxxflags rather than before).

@spectras
Copy link
Author

spectras commented Jul 9, 2024

For the record, I also have the same issue with some other flags. For instance linker flag /GUARD:EHCONT at the profile level cannot be disabled by doing /GUARD:NOEHCONT in the recipe where this feature is known not to work.

@memsharded
Copy link
Member

(I believe that means a way for the recipe to specify that some extra_cxxflags must end up after the tools.build.cxxflags rather than before).

Yes, that is true, the profile always try to have higher precedence over the recipe, otherwise it would be very bad UX, the user trying to define something and being ignored by the recipe.

That means that the order of flags is relevant for msvc in this case, and that defining /sdl /wXXXX is not the same as defining /wXXXX /sdl?

This might be an XY problem. If there is a way to pass the /wXXXX flags from the recipe without the profile-level /sdl overriding them, that would make my initial question moot.

Why not defining /sdl in the recipe too, then the right order would be guaranteed?

@spectras
Copy link
Author

spectras commented Jul 9, 2024

That means that the order of flags is relevant for msvc in this case, and that defining /sdl /wXXXX is not the same as defining /wXXXX /sdl?

Yes /sdl implicitly disables /wXXXX. So /wXXXX must come after /sdl.
msvc strikes again… I know, so many quirks just for that one compiler…

Why not defining /sdl in the recipe too, then the right order would be guaranteed?

  • Not all the profiles we have use /sdl.
  • I guess we could add an option (or a setting? what would be better?) to each recipe and set it based on the option but:
    • It does not scale well (/sdl is not the only one in this situation - I mentioned /guard:ehcont, there are a few others).
    • Though we host a copy of the conan-center recipes we use, we still update from the upstream regularly and the more changes we do the more work fixing conflicts when we update.

That is a possible workaround, though a costly one.

@memsharded
Copy link
Member

Hi again,

I have been re-thinking this, and trying to see if there could be some other approach. My conclusions:

  • It is not possible to define in profiles anything per-package (conf, options, settings) that would be "viral", that is, that it requires the consumers of a package that received that input to have the same input automatically.
  • As initially suggested, having to specify every individual "consumer" package cxxflags in the profiles, doesn't scale
  • The case above is a even more complex case, it is not "viral" per se, but it requires knowledge of the consumers of a package, even if that package is not applying at all the cxxflags, for example, if there are pre-compiled binaries, the cxxflags for that package are not necessary at all as the are used just for the build from source, however they just become necessary when using /sdl in the consumer of that package.

Trying to define that in profiles via raw cxxflags doesn't seem a good approach.

Not all the profiles we have use /sdl.
I guess we could add an option (or a setting? what would be better?) to each recipe and set it based on the option but:
It does not scale well (/sdl is not the only one in this situation - I mentioned /guard:ehcont, there are a few others).

This seems indeed in the good direction, trying to provide a higher abstraction over this. Maybe the /sdl doesn't affect the binary output, those are just checks, but it is very possible that /guard:ehcont and other flags produces an actually different binary. In that case the recommendation is to make them part of the binary model. In this case, it could be a hardened subsetting for the msvc compiler for example, and the msvc could be built with and without hardening, if this is what you mean with not all profiles use /sdl (if all msvc use it, then I would just condition it to if is_msvc(self): in the recipes and move forward.

If it is a subsetting or an option, really depends on the number of packages that uses this, I think it sounds better as a subsettting, but I don't have enough knowledge of your scenario.

Though we host a copy of the conan-center recipes we use, we still update from the upstream regularly and the more changes we do the more work fixing conflicts when we update.

I don't think this would be a big issue. The if ... to protect the definition of these flags will be quite local and independent, it is not that likely that it will produce conflicts that easily.

The local-recipes-index feature and the recommendations to work from a fork of conan-center-index is exactly because there are things that need to be customized at the recipe level as they cannot be defined in external profiles only.

How many packages are we talking about?
Because being pragmatic, if you have dependendency graphs of a few hundred packages, and just a handful of them requires the exceptions, then maybe maintaining a explicit definition in profiles of:

# Note, no addition, just definition
package_/*:tools.build:cxxflags=["/sdl", "/w24146"]
package_b/*:tools.build:cxxflags=["/sdl", "/w24146", "/w24308"]
...

would be enough? Using jinja templates it might be easy to just define lists of packages to exclude for every /wXXXX exclusion and iterate over them, I think it can be done with just a few lines, even for large number of deps, even if inconvenient to define the first time it would be just a relative small effort to be done once. To improve maintenance of that profile, it can be extracted and "included" or composed.

@spectras
Copy link
Author

Thanks for taking the time to think about this. About the flags:

  • /sdl indeed affects the ABI.
  • /guard:ehcont adds metadata, which is consumed during the final link step. Thus:
    Consumer Consumed static library Links
    /guard:ehcont
    /guard:ehcont
    /guard:ehcont /guard:ehcont
  • We also use /arch:AVX2 and /guard:cf.

In fact, given how many flags impact the ABI, after carefully considering our options we defined:
tools.info.package_id:confs = ["tools.build:cflags", "tools.build:cxxflags", "tools.build:defines"] in our global configuration.

This is not ideal as any cxxflags or cflags is now part of the binary model. But it's definitely easier than altering every single recipe to add extra with_ehcont, with_cf, … options for each of those.

We also considered making those different build_type but we would face combinatory explosion there.


If it is a subsetting or an option, really depends on the number of packages that uses this, I think it sounds better as a subsettting, but I don't have enough knowledge of your scenario.

We did not think about a subsetting. The design intent of subsettings vs options is not really clear to us. So… so far we only introduced subsettings when they could be attached to an existing one. For instance we defined a os.glibc subsetting for Linux.
We would still need to modify all recipes to add some form of if is_msvc(self) and self.settings.whatever: tc.extra_cxxflags += […]

Because being pragmatic, if you have dependendency graphs of a few hundred packages, and just a handful of them requires the exceptions, then maybe maintaining a explicit definition in profiles

That's possible, but this needs to be duplicated into every profile, which is not ideal. We have… too many profiles :)


At this point I don't think this deserves immediate attention. It seems this falls in a blind spot of the current model, and it would make more sense to keep this around for consideration for conan3 down the line. Zooming out, at the core, our problem is that:

  • the profile needs to set global cxxflags.
  • those cxxflags cause build errors, and solving those requires package-specific cxxflags.

In the current model, as profile always trumps package flags, the only solutions are lifting package-specific things to the profile, or implementing profile-level changes in packages, both being cumbersome and error-prone.

On the other hand, maybe some form of explicit tc.override_profile_cxxflags = [...] would be enough.

@memsharded
Copy link
Member

Many thanks for the feedback.

We did not think about a subsetting. The design intent of subsettings vs options is not really clear to us. So… so far we only introduced subsettings when they could be attached to an existing one.

This sounds to me that is kind of a subsetting for the compiler=msvc compiler, so compiler.hardening or compiler.myconf, which could have a null to represent the default and introduce it as non breaking. If you did for os.glibc for Linux this seems similar case, just for msvc compiler.

It will also avoid having to set the tools.info.package_id:confs for adding there the flags explicitly.

That's possible, but this needs to be duplicated into every profile, which is not ideal. We have… too many profiles :)

But not really. Profiles admit composition via include(otherprofile), also via command line -pr=profile1 -pr=profile2. In the first case, it might even be done conditional to some jinja templating, or there are also other jinja inclusion mechanisms.

@memsharded
Copy link
Member

This was closed by #16639

The test checks basic test_package conf application, but that wasn't really enough for the above.

Still, it seems that the best way to model those cases was something like a compiler subsetting to build a proper abstraction over it, so please feel free to re-open to continue the conversation, or you can always create a new ticket for any further question. Many thanks for your feedback!

@spectras
Copy link
Author

Unfortunately I did not have time to spend on this. We don't have dedicated devops people here and this happens a bit in-between, it's difficult to justify spending time on something we have a workaround for.

I will re-open if/when this comes back in focus. In the perfect world this just lingers until conan3, and that hypothetical conan3 makes it moot. In the meantime, thanks for your work, conan was a cornerstone for us moving from hand-written scripts to a more declarative and reliable approach to dependencies.

@memsharded
Copy link
Member

Unfortunately I did not have time to spend on this. We don't have dedicated devops people here and this happens a bit in-between, it's difficult to justify spending time on something we have a workaround for.

Totally understandable, don't worry about that.

I will re-open if/when this comes back in focus. In the perfect world this just lingers until conan3, and that hypothetical conan3 makes it moot. In the meantime, thanks for your work, conan was a cornerstone for us moving from hand-written scripts to a more declarative and reliable approach to dependencies.

Happy that Conan has been valuable to you, thanks very much for the feedback, it is really appreciated!!! 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants