search

nmeum / android-tools

Unoffical CMake-based build system for android command line utilities
Apache License 2.0
191 stars 57 forks source link

Does fastboot still require libpcre? #41

Closed mid-kid closed 3 years ago

mid-kid commented 3 years ago

What it says on the tin. It seems to link and function fine without pcre2-8 in the target_include_libraries.

mid-kid commented 3 years ago

Same with fmt::fmt. I don't even have that installed.

Johnnynator commented 3 years ago

pcre2 is used by selinux/libselinux/src/regex.{c,h}, but all includes of regex.h that are used in this subset, are mostly no-op-ish, but maintaining a patch set to keep this clean is probably more work, than just requiring it at compile time.

The fmt::fmt seems to actually not be used anymore in any of our used code parts, so it should be safe to drop.

mid-kid commented 3 years ago

I see. Then libpcre is just a build-time dep, just like gtest. I wonder if providing a mock header would work as well, here.

sgn commented 3 years ago

Isn't fmt::fmt used in core/fastboot/vendor_boot_img_utils.cpp?

nmeum commented 3 years ago

vendor/libbase/include/android-base/format.h should also require libfmt. I think neither pcre2 nor libfmt can be removed. I don't think mocking headers are worth the effort.

mid-kid commented 3 years ago

But libfmt isn't even installed and it builds/runs fine... Keep in mind that not all of the files included within the vendor folder are actually used by the build system. Also it feels silly to require a dependency that never actually gets linked/used during runtime, though pcre isn't a massive deal I guess.

nmeum commented 3 years ago

Which version of android-tools are you referring to? Git HEAD? Because git HEAD does not build without fmt:

/home/nmeum/src/android-tools/vendor/libbase/include/android-base/format.h:23:10: fatal error: fmt/chrono.h: No such file or directory
   23 | #include <fmt/chrono.h>
      |          ^~~~~~~~~~~~~~
compilation terminated.

platform-tools-31.0.0p1-r1 does not require fmt though.

mid-kid commented 3 years ago

I was referring to the latest github release, but thanks, good to know.

nmeum commented 3 years ago

Ah, the fmt dependency is about to be introduced with the upcoming platform-tools-31.0.2 release. Next time just mention which version you are referring to :)

mid-kid commented 3 years ago

For some reason I'm building 31.0.2 without libfmt without issues still... There's no fmt/chrono.h in my /usr/include EDIT: Nvm, just noticed it's vendored.

nmeum commented 3 years ago

Yes, because it is vendored now, see #40.