-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[autoconf] add conan v2 support #12777
Conversation
Note: test_package can't use the new Autools generator due to conan-io/conan#11975
This comment has been minimized.
This comment has been minimized.
@uilianries thanks for the review. I'm a bit uncertain how to handle the I will give it some extra thought in the evening as well |
From review comment
This comment has been minimized.
This comment has been minimized.
According review comment
According review comment
This comment has been minimized.
This comment has been minimized.
KB-H013 states that everything needs to be installed under bin for tools. This also means that the `PATH` for the actual bin location `bin/bin` needs to be added to the `build_env`
This comment has been minimized.
This comment has been minimized.
This PR is also dependent on #12815 |
This comment has been minimized.
This comment has been minimized.
subsystem isn't known at this time
No need to actually compile something with this tool we simply need to know if it runs. The win_bash for the run needs to be set explicitly due to the conf not working properly in the test_package see conan-io/conan#11975
This comment has been minimized.
This comment has been minimized.
# KB-H013 we're packaging an application, hence the nested bin | ||
bin_dir = path.join(self.package_folder, "bin", "bin") | ||
self.output.info(f"Appending PATH env var with : {bin_dir}") | ||
self.buildenv_info.prepend_path("PATH", bin_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.buildenv_info.prepend_path("PATH", bin_dir) |
Did I not review this part already? It's useless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you did review that part, but that was on account of the everything placed in the layout folders. Like a regular package. Since this recipe is a build tool (application) all the folders are placed in the bin folder.
So basically the folder containing the actual binaries is self.package_folder/bin/bin
since the layout bindirs only add the self.package_folder/bin
to the path, I still needed to add this embedded bin dir to the buildenv_info PATH variable.
I tried to solve this with specifying the layout structure, while adhering to kb-h013 but that caused more problems then simply adding this line
self.cpp_info.libdirs = [] | ||
self.cpp_info.includedirs = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happened to these 2 lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The layout method should take care of that as I understand it. If I understand it wrongfully I can add them back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
layout gives a non-empty default value to libdirs & includedirs. Moreover I don't like the idea to control empty libdirs or includedirs in layout() method, it's easier for maintenance to keep all the consuming logic in package_info().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I remove the layout method then?
It is my experience that this will cause issues in recipe (from source). It also adds some extra arguments to the get in the source method. Atleast the documentation provides this as an example to be changed between V1 and V2
Or is it enough to keep the layout and revert the deletion of those two lines?
for src in self.exports_sources: | ||
shutil.copy(os.path.join(self.source_folder, src), self.build_folder) | ||
self.run("{} --verbose".format(os.environ["AUTOCONF"]), | ||
win_bash=tools.os_info.is_windows, run_environment=True) | ||
self.run("{} --help".format(os.path.join(self.build_folder, "configure").replace("\\", "/")), | ||
win_bash=tools.os_info.is_windows, run_environment=True) | ||
autotools = AutoToolsBuildEnvironment(self, win_bash=tools.os_info.is_windows) | ||
with self._build_context(): | ||
autotools.configure() | ||
autotools.make() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep it, with conan v2 flavor please, we must test a build
and conan v1 flavor in test_v1_package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware of the test_v1_package, I will add that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honest question asked out of curiosity; What is the added value of actually compiling something in the test_package, isn't it enough to just validate if the binaries run? I had this discussion with @memsharded earlier and I can think of arguments for both sides. I'm personally inclined to accept a test which just asserts if the binary runs for an application/tool.
On a separate note, please be aware that we can't actually test the Conan v2 on Windows, since issue conan-io/conan#11975
Prevents running autotools
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These autotools recipes are important in conan-center, many recipes depend on them for autotools build. This recipe must check that a basic autotools project builds fine.
If you test only a toy usage of this binary, you may miss issues in the recipe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I understand stand reasoning very well. I will revert the V1 functionality tomorrow morning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it's not acceptable, there are recipes depending on a functional autoconf recipe on Windows. No need to rush, wait msys2 PR. Actually I was planning to submit PR to m4, autoconf, automake and libtool (in this order) afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For us the failing of these chains of recipes (msys2, m4, autotoconf, autotool, gettext, libgettext, cpython, qt) can't be build on Windows with the bash subsystems. These are blocking issues for us at the moment,
It would be ashame if we need to host so many recipes on our own Artifactory server because the CCI versions don't work. It would feel as a big loss for me if the work done on these recipes can't be contributed back to the community and if we both need to maintain parallel recipes for the same dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it's not acceptable, there are recipes depending on a functional autoconf recipe on Windows. No need to rush, wait msys2 PR. Actually I was planning to submit PR to m4, autoconf, automake and libtool (in this order) afterwards.
What would be the main difference between how you would do these recipes and how I made them? I'm willing to put in some extra work to ensure that they're up to the standards of you guys. As I said before it would be a shame if this current effort goes to waste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to rush, wait msys2 PR.
Ah wait I think some information might have gotten snowed over in all the noise.
The conf issue that I mentioned is still an issue even with your updated msys2 recipe. That is the one I'm using on our Artifactory server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SpaceIm I have send you a message on Slack, pls feel free to responsd (or not ;-) ) in your own time
This comment has been minimized.
This comment has been minimized.
It keeps failing for in the v2 test_package with see conan-io/conan#11975:
This is when I run the I unfortunately can't spend anymore time in fixing this for the CCI and I will just have to settle with fixing it for our recipes and hosting it on our Artifactory server. @SpaceIm don't wait for me with these PR's. I tried but I think you will stand a better change of updating them then I do. |
Failure in build 13 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
Superseded by PR #12896 will close this one in favor of that PR |
Note: test_package can't use the new Autools generator
due to conan-io/conan#11975
Specify library name and version: lib/1.0
This is also a good place to share with all of us why you are submitting this PR (specially if it is a new addition to ConanCenter): is it a dependency of other libraries you want to package? Are you the author of the library? Thanks!