-
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
[automake] Added support for conan v2 #12834
Conversation
Use modern Autotools toolchain and add buildenv_info Simplified test_package No need to actually compile something with this tool we simply need to know if it runs. The win_bash for the run in the test_package 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.
subsystem isn't know at this time
@lru_cache(1) | ||
def _autotools(self): | ||
autotool = Autotools(self) | ||
autotool.configure() | ||
autotool.make() | ||
return autotool |
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.
please follow the same patterns than other AutotoolsToolchain recipes in CCI.
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 copied this pattern from Another recipe. Can you point toward an example so I can follow that pattern
def package(self): | ||
autotools = self._autotools() | ||
# KB-H013 we're packaging an application, place everything under bin | ||
autotools.install(args=[f"DESTDIR={unix_path(self, path.join(self.package_folder, 'bin'))}"]) |
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.
same here
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.
Do you mean the specifications of the destdir? This was the only way for me to make sure that the Autotools used the correct Unix path when run on windows with a subsystem.
Also needed because the whole tool is Installed in its entirety in the bindirs.
# 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 environment variable:: {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) |
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.
Points towards a nested bin
dir inside the bindirs
therefore not automatically added. Might be better if I renamed this variable, although i'm not immediately sure what name it should have.
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.
But why didn't you keep the previous layout in package folder? Anyway it's not the conan v2 way to add paths to PATH.
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.
KB-H013 state:
If you are packaging an application put all the contents inside the bin folder.
And the "old" recipe did that as well I believed. Including the resources etc.
self.user_info.compile = os.path.join(self._automake_libdir, "compile") | ||
self.user_info.ar_lib = os.path.join(self._automake_libdir, "ar-lib") | ||
def package_info(self): | ||
# KB-H013 we're packaging an application, hence the nested bin |
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.
not sure comment is useful, maintainers know 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.
The audience is more then just maintainers. You also have semi layman maintainers such as I, with just enough knowledge to mess things up. See the first attempt of my Autoconf PR, were I treated that recipe like a regular run recipe and not a build application.
I just recently came to this knowledge, because of a linter warning in the PR for my Autoconf recipe. while I have been wondering for over half a year what was "wrong" with the cpython recipe because everything was placed in the the bin
dir for that as well.
Just give the word if the comment does more harm then good and I will remove it.
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.
Actually the comment is wrong, there is no reason to nest bin folder.
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.
is the comment wrong or the linting warning?
In other words: would the package_folder
for an application have to follow layout
package_folder
|
|- bin
|- share
|- libs
or (as far as I understand it by reading KB-H013) and implemented it
package_folder
|
|- bin
|- bin
|- share
|- libs
for var in [("ACLOCAL", path.join(self.package_folder, "bin", "aclocal")), | ||
("AUTOMAKE_DATADIR", self._datarootdir), | ||
("AUTOMAKE_LIBDIR", self._automake_libdir), | ||
("AUTOMAKE_PERLLIBDIR", self._automake_libdir), | ||
("AUTOMAKE", path.join(self.package_folder, "bin", "automake"))]: | ||
self._set_env(*var) |
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, your function _set_env always relies on define_path
, it's not what we want for ACLOCAL.
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 have limited experience with Autotools in general except for my current attempts to make these recipes work on Windows with msys2.
Could you elaborate a bit why ACLOCAL should not use define_path?
Isn't it just the path to a single binary?
test_type = "explicit" | ||
win_bash = True |
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.
to remove when msys2 fixed
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.
Will add a fixme comment here, is there an issue nummer which I can reference?
def _build_autotools(self): | ||
"""Test autoreconf + configure + make""" | ||
with tools.environment_append({"AUTOMAKE_CONAN_INCLUDES": [tools.unix_path(self.source_folder)]}): | ||
self.run("{} -fiv".format(os.environ["AUTORECONF"]), win_bash=tools.os_info.is_windows) | ||
self.run("{} --help".format(os.path.join(self.build_folder, "configure").replace("\\", "/")), win_bash=tools.os_info.is_windows) | ||
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 the build please, translate it to conan v2 helpers
Also, test_v1_package is missing here
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.
Will add a test_v1_package.
Please be aware that Conan v2 is difficult to test on Windows with a subsystemen due to issue conan-io/conan#11975
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.
if it doesn't work on window, then PR shouldn't be merged.
This comment has been minimized.
This comment has been minimized.
Failure in build 4 (
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 #12898 I will close this one in favor the new PR |
Use modern Autotools toolchain and add buildenv_info
Simplified test_package No need to actually compile
something with this tool we simply need to know if it
runs.
The win_bash for the run in the test_package needs to be
set explicitly due to the conf not working properly in
the test_package see conan-io/conan#11975
This PR is dependent on: