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

upgraded to importlib #10127

Merged
merged 4 commits into from
Dec 9, 2021
Merged

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Dec 3, 2021

Includes the invalidate_caches() call

Close #4687
Close #3441
Close #6633

@Minimonium this should address previous concerns about python loading, note invalidate_caches() is called in the constructor of ConanFileLoader(), a new one will be created with each ConanApp() which is now explicitly created inside each api call, so I'd say we are ok now.

@memsharded memsharded added this to the 2.0.0-alpha2 milestone Dec 3, 2021
@memsharded memsharded marked this pull request as ready for review December 5, 2021 00:28
@Minimonium
Copy link
Contributor

Great! 😄

Be aware of the pre 3.7.0 bug in invalidate_cache tho!
https://github.com/conan-io/conan/pull/6633/files#diff-c15f4aacf65aa58b31a041bac63e4dd7337355d9ec44a8f32b2a5815d5eb5ea9R210

@memsharded
Copy link
Member Author

Thanks for the pointer to https://github.com/conan-io/conan/pull/6633/files#diff-c15f4aacf65aa58b31a041bac63e4dd7337355d9ec44a8f32b2a5815d5eb5ea9R210

But I have some questions:

  • Shouldn't the clean of the sys.path_importer_cache be done added (instead of replacing) the invalidate-caches()?
  • It would be great to have a failing test for this bug. The test provided in your previous PR doens't make sense anymore because we removed the global importing of files from the cache. Could you provide some hint I could write a test that fails for this?

Thanks!

@Minimonium
Copy link
Contributor

It's been quite a long time since then, so I may miss the fine details, sorry about that.

I'm pretty sure the only reasoning to branch them out was the core issue of the lack of invalidation of the "negative cache". At the time it was "either a small-scope hack or a conventional tool" for the same purpose. I think before opt-in to either there must be a clear understanding of use-cases where it does help.

The issue at the time was specifically with cache reuse inside a single Python invocation, a call to user code in the middle that introduces cache side-effects like conan export ., specifically the "negative cache" issue. An example of that was a naive implementation of the automatic config installation because it contained user-provided hooks, which are modules that Conan explicitly did load.

@memsharded
Copy link
Member Author

Ok I see.

I think the best would be to write some tests when the config install functionality is moved to the new PythonAPI, then we will be able to write tests exactly as the potential user would do, alternating config install calls for hooks with other calls in the PythonAPI. That will surface potential real issues there.

As this is already looking as some progress, I'd probably don't wait to merge it till then, I think this could be merged asap.

conans/client/loader.py Outdated Show resolved Hide resolved
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