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

Raise error when a generator is defined in generators and generate() #12722

Merged

Conversation

AbrilRBS
Copy link
Member

This used to be a mistake, but no error was ever given to the user

Changelog: (Feature): Raise an error when a generator is both defined in generators attribute and instantiated in generate() method
Docs: Omit

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.

…d in generate()

This used to be a mistake, but no error was ever given to the user
@AbrilRBS AbrilRBS requested a review from memsharded December 16, 2022 18:13
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.

Beware, it happens that the VirtualBuildEnv and VirtualRunEnv are implicit, even if they are not declared, they are called after generate().

I think that is the thing breaking the tests.

@AbrilRBS
Copy link
Member Author

AbrilRBS commented Dec 16, 2022

Yeah, I had some errors locally with integration tests that looked like remote issues, and this slipped thru the cracks when checking for real errors.

Most of the were something like this, let's discuss it tomorrow:

Example of one of the test errors I'm getting, click to expand
E           Exception: 
E           ------------------------ Command failed (unexpectedly): ------------------------
E           upload * --confirm -r default
E           ----------------------------------- Output: ------------------------------------
E           Checking which revisions exist in the remote server
E           Checking which revisions exist in the remote server
E           Preparing artifacts to upload
E           Compressing recipe...
E           Compressing package...
E           Please log in to "default" to perform this action. Execute "conan user" command.
E           Remote 'default' username: Please enter a password for "admin" account: ERROR: b"module 'jwt' has no attribute 'encode'"
E           
E           Invalid server response, check remote URL and try again. [Remote: default]```
</details>

@@ -105,6 +105,10 @@ class XcodeDeps(object):
""")

def __init__(self, conanfile):
if self.__class__.__name__ in conanfile.generators:
Copy link
Contributor

Choose a reason for hiding this comment

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

it's duplicated multiple times, maybe it makes sense to extract code to its own function?

Copy link
Member

Choose a reason for hiding this comment

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

I thought that this would take just 2 lines, the if and then 1 line for the error message, not 3.
So @RubenRBS do whatever you think it is best, I am happy with either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

My only issue with this is the thing we mentioned on friday that it's not obvious where to place such helper function. I've done so in tool's init, but happy to change. Thanks @SSE4 for your suggestion :)

@memsharded memsharded added this to the 2.0.0-beta7 milestone Dec 19, 2022
@AbrilRBS
Copy link
Member Author

I've noticed that VirtualRunEnv and VirtualBuildEnv are instantiated in lots of places just to handle some envs, even though the generator is defined in the attribute.
Maybe it would be better for the check to run in the generate() method of at least those generators, and possible the rest too?

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.

Importing a private function from another module can be a linting or style error, we might need to rename it (later, not now), and if there are concerns of being accessible for users, it should be moved to internal namespace.

conan/tools/__init__.py Outdated Show resolved Hide resolved
conan/tools/__init__.py Outdated Show resolved Hide resolved
@SSE4
Copy link
Contributor

SSE4 commented Dec 20, 2022

Importing a private function from another module can be a linting or style error, we might need to rename it (later, not now), and if there are concerns of being accessible for users, it should be moved to internal namespace.

yes, I think conan.tools is a public namespace for recipe authors. if function isn't supposed to be used in recipes, it should go to some other place. also we usually use more specific nested namespaces, like conan.tools.xxx.
leading underscore implies private function, so it's okay.

AbrilRBS and others added 4 commits December 20, 2022 15:26
Co-authored-by: James <memsharded@gmail.com>
Co-authored-by: James <memsharded@gmail.com>
Co-authored-by: James <memsharded@gmail.com>
@memsharded memsharded merged commit ad6cca4 into conan-io:develop2 Dec 20, 2022
@AbrilRBS
Copy link
Member Author

Thanks for your suggestion @SSE4. I ended up adding it to tools/internal as suggested

@AbrilRBS AbrilRBS deleted the rr/error-on-duplicated-generator branch December 23, 2022 11:46
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.

3 participants