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

Add 'CONAN_V2_MODE' to start testing Conan v2 deprecated features #6490

Merged
merged 46 commits into from Mar 4, 2020

Conversation

jgsogo
Copy link
Contributor

@jgsogo jgsogo commented Feb 7, 2020

Changelog: Feature: Add environment variable 'CONAN_V2_MODE' to enable Conan v2 behavior.
Docs: conan-io/docs#1578

If the environment variable CONAN_V2_MODE is defined, Conan will work as Conan v2 (still to provide a roadmap/deprecation summary in a public webpage).

This PR takes care of this breaking changes:

  • default settings.yml:
    • first level settings cppstd is removed
  • default conan.conf:
  • scm_to_conandata activated by default
  • GCC >= 5 autodetected libcxx is libstdc++11
  • Forbid access to self.cpp_info in conanfile::package_id()
  • Deprecate conanfile::config()
  • Deprecate old python_requires syntax (error raises if it is used, not only if it is imported)
  • Forbid access to self.info in conanfile::package()
  • default_options required to be a dictionary type
  • Raise if scopes in profile
  • Raise if setting cppstd in recipe
  • Forbid self.settings and self.options in conanfile::source() method
  • Deprecate tools.msvc_build_command
  • Deprecate tools.build_sln_command
  • Deprecate cpp_info.cppflags
  • Deprecate environment variables CONAN_USERNAME and CONAN_CHANNEL

It adds several tests to check that the previous errors are actually being raised.

The idea is to inherit these tests from ConanV2ModeTestCase class which is setting the proper variable in the environment. Also, tools like TestClient will be created by this class (for these tests) to ensure that the defaults for v2 are being set.

#REVISIONS: 1

@jgsogo
Copy link
Contributor Author

jgsogo commented Feb 7, 2020

@memsharded, wdyt about this approach?

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.

Looking good, keep going!

conans/test/utils/conan_v2_tests.py Show resolved Hide resolved
@jgsogo jgsogo marked this pull request as ready for review February 26, 2020 09:03
@jgsogo jgsogo requested a review from czoido February 26, 2020 09:04
@jgsogo
Copy link
Contributor Author

jgsogo commented Feb 26, 2020

I think this is enough for the first iteration.

Docs are pending: probably we need a dedicated section in the docs to explain the roadmap to v2 and this environment variable, deprecated features, new defaults,... agree?

@jgsogo jgsogo added this to the 1.23 milestone Feb 26, 2020
Co-Authored-By: Carlos Zoido <mrgalleta@gmail.com>
Copy link
Contributor

@SSE4 SSE4 left a comment

Choose a reason for hiding this comment

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

LGTM. the only thing is maybe the name conan_v2_behavior is not that descriptive and too generic. maybe something like check_v2_compatibility would be the better naming?

@jgsogo
Copy link
Contributor Author

jgsogo commented Mar 4, 2020

LGTM. the only thing is maybe the name conan_v2_behavior is not that descriptive and too generic. maybe something like check_v2_compatibility would be the better naming?

I'll keep the conan_v2 as it is almost a keyword to search the codebase, but I agree that the behavior part can be improved. Let's wait for @memsharded review before adding more changes.

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.

Looking good, great job. A couple of things:

  • The test_proper_usage tests reads as highly redundant
  • Always skipped tests, better to keep them for later
  • The conan_v2_property should be no-op without CONAN_V2_MODE env-var, I see that you want to print warnings for v1, but it has a risk that it is not worth it.

conans/client/conf/detect.py Show resolved Hide resolved
conans/client/graph/graph_builder.py Show resolved Hide resolved
conans/client/graph/graph_builder.py Outdated Show resolved Hide resolved
conans/client/loader.py Outdated Show resolved Hide resolved
conans/util/conan_v2_mode.py Outdated Show resolved Hide resolved
conans/test/conan_v2/test_default_config.py Show resolved Hide resolved
conans/test/conan_v2/settings_model/test_xbuild.py Outdated Show resolved Hide resolved
conans/test/conan_v2/conanfile/test_source_method.py Outdated Show resolved Hide resolved
conans/test/conan_v2/conanfile/test_package_method.py Outdated Show resolved Hide resolved
conans/client/conf/__init__.py Show resolved Hide resolved
@jgsogo
Copy link
Contributor Author

jgsogo commented Mar 4, 2020

A comment about the two last points:

  • revisions_enabled: last time we agree that Conan v2 (compatibility mode) could keep working without revisions. We can add more things to this CONAN_V2_MODE in the future, in next 1.x releases and there will be plenty of time to remove support for "revisions disabled" (maybe remove the old APIv1). For this PR I think it is enough.
  • conan_v2_property I understand the risk, but I'd really like to keep it, it will print the warning, otherwise only people that activate the CONAN_V2_MODE will realize about this wrong usage of Conan. The magic is only applied around very specific class methods, all our tests are green,... it could be as risky as the implementation of the base class for the python_requires.

@memsharded
Copy link
Member

revisions_enabled: last time we agree that Conan v2 (compatibility mode) could keep working without revisions. We can add more things to this CONAN_V2_MODE in the future, in next 1.x releases and there will be plenty of time to remove support for "revisions disabled" (maybe remove the old APIv1). For this PR I think it is enough.

Ok, agree.

conan_v2_property I understand the risk, but I'd really like to keep it, it will print the warning, otherwise only people that activate the CONAN_V2_MODE will realize about this wrong usage of Conan. The magic is only applied around very specific class methods, all our tests are green,... it could be as risky as the implementation of the base class for the python_requires.

The thing is that users will certainly not read those warnings, and they will not take action to remove those warnings. It is a nice to have, but I think the real effect on awareness will be very little.

The risk here is users doing unusual things with python, like adding class members inside methods, or weird things over the attribute itself. I really think there is a non small risk of breaking, and given that we are starting to plan for Conan 2.0, these nice to have, informational warnings are not worth.

@jgsogo
Copy link
Contributor Author

jgsogo commented Mar 4, 2020

Waiting for the CI, but I'll start with the docs

@memsharded memsharded merged commit fc55b1c into conan-io:develop Mar 4, 2020
@jgsogo jgsogo deleted the conanv2/try-catch branch March 5, 2020 07:40
@jgsogo jgsogo mentioned this pull request Jun 19, 2020
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.

None yet

4 participants