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] Conan 2 rejects upload due to manifest mismatch due to __pycache__ #14838

Closed
1 task done
marlamb opened this issue Sep 28, 2023 · 8 comments · Fixed by conan-io/conan-center-index#21764
Closed
1 task done

Comments

@marlamb
Copy link
Contributor

marlamb commented Sep 28, 2023

What is your question?

Hi! I am currently migrating towards Conan 2.x and saw an unexpected behavior while trying to build and upload the package meson from the conan-center-index with only one adaptation from my side, using a different version of ninja.
I am using the Conan version 2.0.12 in a Python environment of version 3.10.13.

From the previous behavior of Conan 1.x and after reading the issues #7789 #4961 and #5404 I was under the impression that Conan deliberately ignores cache files (which makes sense in my opinion).
However, for Conan 2 it complains during the upload about a manifest mismatch and lists then all files residing in a __pycache__, which are created during the execution of the test_package.

I am using the following commands

conan create <path_to_conanfile> --user me --channel testing --format json
conan upload <ref:package_id#prev> --check --confirm --remote <my_remote>

My question is: Is this behavior for Conan 2.x expected, e.g. by a different default workflow (e.g. not executing the test_package before an upload - which would feel wrong to me)? Or is this some kind of degradation compared to the behavior of Conan 1.x?

Have you read the CONTRIBUTING guide?

  • I've read the CONTRIBUTING guide
@memsharded
Copy link
Member

Hi @marlamb

Thanks for your question.

Neither Conan 1.X or Conan 2.0 should create __pycache__ files, because the bytecode file generation is disabled for conanfiles. I guess there is something else ongoing there to create these files, but Conan shouldn't generate them either in the cache nor in the user folders.

If things are changing in the manifest, maybe it could be some wide export pattern that is exporting also the contents of the test_package folder? That behavior should also be the same in 1.X and 2.0, this is not intended in any case ,and export patterns should not include the test_package folder, specially when it can be dirty with temporary build files.

It is true that Conan 2.0 is no longer filtering pyc and pycache files, that is, the CONAN_KEEP_PYTHON_FILES=1 env-var behavior has been made the default. This was done because there were a bunch of users wanting to create .pyc files, and being blocked because of Conan not packaging them. It made little sense for Conan to be smart about these files if it was no longer creating them (the initial behavior made sense when Conan was not disabling its own .pyc/pycache generation for its own python files).

You might use the exclude patterns or the export() method for more control over what gets exported (you can use it also in 1.X, the goal is that the same recipe is compatible both in 1.X and 2.0)

Please let me know if this helps.

@marlamb
Copy link
Contributor Author

marlamb commented Sep 28, 2023

Hi @memsharded , thanks for the fast reply.
As far as I can see the meson recipe does not export anything. But it uses copy(self, pattern="*", src=self.source_folder, ...) within the package step. So no chance to improve the problem on that side. Just to be sure I tested it with excludes="*.pyc", but it did not work.

The environment variable sounded promising, but unfortunately it did not work out as hoped 😞 . What I did is simply

export CONAN_KEEP_PYTHON_FILES=0
conan create ...
conan upload ...

Or is it meant to be used differently? If I got your comment correctly it would have been expected that setting the env variable should enable filtering in the package step. But for me it looks like the filtering (at least for Conan 1.x) is also active during the upload.

However, I am not sure if we are talking about the exact same behavior. What I can see is the following:

  • After package step no __pycache__ exists within the package.
  • After execution of the test_package, which runs multiple Python commands using the package, __pycache__ directories containing .pyc files are created within the package in my local Conan cache.
  • Conan 1 (--check --confirm --no-overwrite all) can upload the package despite the additional files, Conan 2 (--check --confirm) rejects during the integrity check.

What actually works is to set

export PYTHONDONTWRITEBYTECODE=1
conan create ...
conan upload ...

but I would consider this as a non-optimal solution as IMO it is preferred to use the byte code to improve Pythons execution speed, so users should not rely on deactivating it within their terminals.

Putting it into the Environment in an added generate step of the test_package works as well.

Summarizing: Some ways exist to solve my problem 😃 . But the default settings combined with recipes from the conan-center-index don't seem to work without special tweaks. And still the behavior of Conan 1 seems to be different to Conan 2 with regard to the integrity check (the former ignoring existing .pyc files the latter complaining).

I am looking forward to read your thoughts about it 👍 .

@memsharded
Copy link
Member

The CONAN_KEEP_PYTHON_FILES=1 is an opt-in in Conan 1.X exclusively to activate the future 2.0-compatible behavior of not automatically filtering the .pyc files.

Thanks for the details, I have managed to reproduce. It indeed makes sense:

  • Meson is inside a conan package
  • When meson is executed it creates new pycache folders and files
  • Conan packages have a manifest, and those pycache files are not to be uploaded, they could fail in another machine
  • This is not a big issue in ConanCenter, because ConanCenter first uploads the package, then call conan test after, for some CI reasons
  • Conan 2.0 is totally correct about not allowing the upload, those files cannot be there at the upload time, they could break the package. The goal is that the recipe doesn't have the files there.

I think this is something to try to fix in the Meson recipe in conan-center-index, but I am going to label this as "look-into" for discussion with the team, and check possible approaches. It might not be easy, in the meantime, I'd suggest:

  • Run conan create . -tf="" in Conan 2.0
  • Run conan upload ...
  • Run conan test test_package ... to test the pacakge.

@marlamb
Copy link
Contributor Author

marlamb commented Sep 28, 2023

The CONAN_KEEP_PYTHON_FILES=1 is an opt-in in Conan 1.X exclusively to activate the future 2.0-compatible behavior of not automatically filtering the .pyc files.

That definitely explains the behavior I saw, I was already wondering why the Conan 2 documentation does not mention the variable 😆 .

Also the rest makes sense to me, but I must admit I am not really happy about it. For me having a test_package is in order to find mistakes I did in the recipe before uploading it to a remote where it is potentially visible to everyone. Performing the upload before test execution just feels wrong to me. And as the test execution is the default by conan create it also looks from a naive perspective like it is intended to always run, which I consider a great and comprehensible interface.
I am looking forward to result of the discussion in your team and hope for a solution that allows the naive conan create ..., conan upload ... command order, which IMO users might expect (at least I do 😉 ). [One possible solution could be to have an automated cleanup step in the test_package, ensuring that it does not alter - or at least not add files to - the state of the package itself. Another partly solution for the very specific problem with the .pyc files could be to make PYTHONDONTWRITEBYTECODE=1 a default within the test package build and run environments.]

What I still don't understand is: for Conan 1 the upload just works fine and the .pyc files are not part of the package on the remote. Note that also here the files were created after the packaging step by the test_package and before the upload. A possible explanation could be that CONAN_KEEP_PYTHON_FILES=0 not only influences the packages, but the upload as well. Is that correct?

@memsharded
Copy link
Member

I am looking forward to result of the discussion in your team and hope for a solution that allows the naive conan create ..., conan upload ... command order, which IMO users might expect (at least I do 😉 ). [One possible solution could be to have an automated cleanup step in the test_package, ensuring that it does not alter - or at least not add files to - the state of the package itself.

Yes, we agree here, the flow must be supported. The problem seems very specific to the nature of Python apps creating extra files in a place that is supposed to be immutable, because it has a manifest that declares the files in the package, and after executing it and creating pycache files, the contents of the package has been changed. We think this needs to be fixed, so the conan create + upload works without issues and extra tricks.

The only thing is that this is not tied exclusively to the test_package. It can also happen without test_package, simply when a package is consumed, used by other packages and re-uploaded. So we need to investigate some solution that is embedded in the package itself, cannot be solved from the test_package only.

@memsharded
Copy link
Member

We have had a look, it seems that something like https://docs.python.org/3/using/cmdline.html#envvar-PYTHONPYCACHEPREFIX would be the way to go. It might have some limitations, like it is Python3.8 only. The approach at this moment would be to define it in the meson wrapper in the recipe.

@memsharded
Copy link
Member

I am submitting this to ConanCenter conan-io/conan-center-index#21764, as a potential workaround for the Meson package recipe.

Also, for the other potentials solutions, the tickets to track are:

@memsharded memsharded removed this from the 2.0.15 milestone Dec 14, 2023
@AbrilRBS
Copy link
Member

We'll tackle the general issue pointed out by this report as part of the #15277 work - please keep an eye on that issue :)

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.

3 participants