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

Rename QbsToolchain to QbsProfile #8537

Merged
merged 3 commits into from
Feb 22, 2021
Merged

Conversation

Psy-Kai
Copy link
Contributor

@Psy-Kai Psy-Kai commented Feb 21, 2021

This generator generates a qbs profile. Renaming the class from Toolchain
to Profile makes this more obvious.

Changelog: Fix: Renamed generator QbsToolchain to QbsProfile.
Changelog: Fix: Renamed default filename of QbsProfile generated file to conan_toolchain_profile.qbs.
Changelog: Fix: Renamed Qbs attribute use_toolchain_profile to profile.
Docs: conan-io/docs#2027

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

Kai Dohmen added 2 commits February 21, 2021 18:12
This generator generates a qbs profile. Renaming the class from Toolchain
to Profile makes this more obvious.
The qbs profiles are names `profile` not `toolchain_profile`. Rename to
make this clear.
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 fine with this change, mainly because you are Qbs maintainers. It is very likely that I would reject this PR otherwise. Even if the feature is experimental, we try to minimize breaking changes. Renaming or other "cosmetic" changes are typically not enough to break.

In this case I trust you that Qbs users will not be very annoyed for this break, but please consider it for future changes.

Many thanks for keeping maintaining it!

@memsharded memsharded added this to the 1.34 milestone Feb 22, 2021
@memsharded memsharded merged commit 55ccd5c into conan-io:develop Feb 22, 2021
@Psy-Kai
Copy link
Contributor Author

Psy-Kai commented Feb 22, 2021

Thanks for making that exception :)
I tried to not break the things for now (thats why I let the from conan.tools.qbs.qbsprofile import QbsProfile as QbsToolchain in there) so other users have time to migrate.

@memsharded
Copy link
Member

I tried to not break the things for now (thats why I let the from conan.tools.qbs.qbsprofile import QbsProfile as QbsToolchain in there) so other users have time to migrate.

Oh, that is true, good point, I skipped that part, well done. It is also possible to create a temporary alias QbsToolchain that print warnings, but not necessary, as it was released recently and the documentation change, it is fine. Thanks!

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.

It seems this change didn't include the renaming of the text generator:

generators = "QbsToolchain"

Would you like the generator for the Qbs toolchain to be called QbsProfile?

@Psy-Kai
Copy link
Contributor Author

Psy-Kai commented Feb 25, 2021

Oh yes, it seems like I forgot something. Do you mean

elif generator_name == "QbsToolchain":
? Or is there another place I forgot to change?

@memsharded
Copy link
Member

Hi @Psy-Kai

Yes, that is the place.

In any case, I am wondering what is the impact on Conan users. All other build sytems will have a XXXXToolchain which is the responsible of creating any files that the build system needs. In CMake it will be find_package scripts, in MSBuild .props properties files... Having this class named differently for Qbs might probably confuse initially Conan users using other build systems too.

Please tell me, because we can still add it to Conan 1.34 before it is released (is already branched)

@Psy-Kai
Copy link
Contributor Author

Psy-Kai commented Feb 25, 2021

I just thought that the "Toolchain" naming was a little off. For Qbs a Profile is generated. So it would be more self explaining if I write something like generator = QbsProfile. The "Toolchain" may be a little bit confusing for Qbs users because in the Qbs Documentation there is no real mention of such thing.

So my intention was self explaining code. And I thought that this "Toolchain"-Generators will be renamed some time because they are just generators (I found comments in the code hinting such a thing). The BuildInfo generators are not called "cmakebuildinfo" too (even if that would be some step towards more self explaining code).

@memsharded
Copy link
Member

Ok, I understand. Our current naming is:

  • XXXDeps: Generator like CMakeDeps that handle dependencies build information (find_packages)
  • XXXToolchain: Generator like CMakeToolchain or AutotoolsToolchain that handle the mapping of Conan settings and options to build system specific , like conantoolchain.cmake or autotoolstoolchain.sh shell script with env-vars. Toolchain is not necessarily a "toolchain" file, is any file that the build system could use to define its build configuration
  • XXXX: The build helper command line wrapper, like CMake
  • XXXXGen (currently under consideration): A high level generator that bundle at least XXXDeps, XXXToolchain and VirtualEnv

I am fine if you want to use the QbsProfile and it will be easier for your users. Please do a PR asap to the release/1.34 branch, fixing the above issue (I can manage too if you can't do it now). Thanks!

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

Successfully merging this pull request may close these issues.

2 participants