conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.15k stars 971 forks source link

Not exporting FindXXX.cmake file with conan test_package #650

Closed piponazo closed 7 years ago

piponazo commented 7 years ago

Hi!

I am not sure if this is a real problem or not, but I find it a bit annoying. I was writing a recipe for a library that is configured with CMake and I am creating my own FindXXX.cmake file. At some point I had the library compiled correctly and the package created at my .conan/data/Lib/Version/User/Channel/package/hash directory for the Release configuration.

At that point I started to do some new changes in the recipe for making it work for the Debug configuration. The first thing that I tried was to modify my the FindXXX.cmake file. What I found a strange is that when I recompile the package with "conan testpackage" the new modified FindXXX.cmake is not placed in the .conan/data/Lib/Version/User/Channel/package/hash_ folder. Actually the new version of the file is not placed in any of the conan foldres (source, export, package, build).

Is that the expected behaviour?

memsharded commented 7 years ago

So you are modifying the recipe, then testing/creating-package for different settings? And the issue is that other packages built for other settings are not updated, or automatically removed?

In that case this is expected. The rationale is that exporting a recipe doesn't necessarily invalidate the binary packages, imagine for example that you added just a comment, or some meta-information of the recipe, but you had already compiled a big library for boost for many configurations, and it took many hours. Having them automatically deleted would be annoying. While creating packages is the responsibility of the package creator to know if the changes require a conan remove to delete the old binaries or not. Does this answer your question? Thanks!

piponazo commented 7 years ago

I have rewritten the original message trying to be more concise.

I am not modifying the settings of the recipe, I am just modifying the file FileXXX.cmake. I was expecting to see the new version of the FileXXX.cmake file in the .conan/data/Lib/Version/User/Channel/package/hash folder after running conan test_package. Actually when I run the command I can see this output:

luis@luis-W740SU:~/programming/conan/conan-openmesh$ conan test_package -k
Exporting package recipe
OpenMesh/4.1.1@piponazo/testing export: Copied 1 '.cmake' files: FindOpenMesh.cmake
OpenMesh/4.1.1@piponazo/testing: The stored package has not changed
Requirements
    OpenMesh/4.1.1@piponazo/testing from local
Packages
    OpenMesh/4.1.1@piponazo/testing:81607951755e61cf17149d40bed6aba5b3a079a9

OpenMesh/4.1.1@piponazo/testing: WARN: Forced build from source
OpenMesh/4.1.1@piponazo/testing: Building your package in /home/luis/.conan/data/OpenMesh/4.1.1/piponazo/testing/build/81607951755e61cf17149d40bed6aba5b3a079a9
OpenMesh/4.1.1@piponazo/testing: Copying sources to build folder

The 2nd line is saying that 1 .cmake file has been copied but I guess that it means other thing, since the .cmake file that I have in the package folder is the old one.

The only way I have to see the new .cmake file in the package folder is to re-do all the work (deleting the .conan/data/Lib/ folder and re-running conan test_package)

lasote commented 7 years ago

@piponazo yes, it looks like a bug. I'm going to investigate.

lasote commented 7 years ago

I think the problem is the "-k" option. It's keeping the old source folder, so it is not copying the findXXX.cmake file from the exported files. Please, try to remove -k to confirm it. Then we can think about this use case, I assume you don't want to download or clone the sources again but you want to override the exported files. right?

piponazo commented 7 years ago

It is even happening when I use the conan test_package command without the -k option :(.

As you guessed, I tried first with -k to avoid the download, but it is even happening without the -k option.

memsharded commented 7 years ago

I'see, I am going to try to reproduce this behavior in a test and I'll contact back, thanks for the feedback!

memsharded commented 7 years ago

Not able to reproduce the issue, I have done a test with the following:

Is this what is failing for you?

piponazo commented 7 years ago

I am sorry guys, I do not know how I was running the commands but I just try to reproduce the issue in my two computers and now I am not able to do it. Luckily, It is working as @memsharded :relaxed:

Anyway ..., with the -k option the FindXXX.cmake file is not exported. @lasote mentioned that it could be an interesting use case to consider. Do you think it could be considered as the default behavior? or it could break some other use cases?

memsharded commented 7 years ago

No worries! Those things happen, thanks for trying and reporting :)

The -k argument means exactly keep the source folder, and the flow of the package creation is always export->source->build/sha->package/sha. I see no way of maintaining the source folder and having the modified FindXXX.cmake progress all the way to the package folder.

Basically, the -k option is useful for common changes in the conanfile.py, because the conanfile is read from the export folder, but is useless if modifying other exported files.

The only way to achieve it would be copy files always from export->source, even if the folder exists, but this would increase time for users not using the source() method, but using the exports to snapshot the source code of the package into the recipe. This might be acceptable (local copies are faster than downloads, and many packages sources are not that big), what do you think @lasote?

lasote commented 7 years ago

I'm not sure. I think if we copy export-> source always some users could be annoyed. I would prefer two other options:

  1. Keep it with the current behavior.
  2. Copy always export->source but introduce an opt-out like a "-ke" to keep also the export. But it add complexity, so I'm not sure.
memsharded commented 7 years ago

Had a look, my thoughts:

So I suggest leaving the current behavior as is, I think adding more options is excessive complexity, not only code but UI for an optimization.

memsharded commented 7 years ago

Hi @piponazo , any new thoughts on this issue? As a summary of my previous thoughts, I'd say that this issue can be closed, or do you have any other suggestion? Thanks very much!

piponazo commented 7 years ago

Hi guys. Sorry for not being responsive the last weeks. I agree with your points, specially with the one mentioning that this new tiny feature would complicate a the UX and it would not give too much benefits. It is just a tiny detail for those of us writing recipes.

Anyway, thanks for taking the time to consider it!

memsharded commented 7 years ago

Dont worry, being very busy is more than usual... Thanks to you for the feedback!