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

Compilation fixes #25

Closed
wants to merge 3 commits into from
Closed

Compilation fixes #25

wants to merge 3 commits into from

Conversation

no92
Copy link

@no92 no92 commented Oct 1, 2022

Compiling current master on Arch Linux fails for two reasons, both of which this PR addresses:

  • argparse.hpp currently does not include <utility>, which results in an compilation error; this is fixed by bumping it to v2.9.
  • Arch Linux ships it clang libraries as clang-cpp (as do some other distros), so that meson can't find the libraries; this is fixed by defaulting to using clang-cpp, but falling back to the old method. This is the same approach as used by mesa.

@hdoc
Copy link
Owner

hdoc commented Oct 1, 2022

Thank you for the contribution, these are both very helpful fixes. It's surprising to hear that argparse doesn't compile on Arch, it must be some kind of distro-specific packaging anomaly. Likewise it's frustrating that we have to work around how different distributions package LLVM/Clang, but thank you for doing the hard work of finding and solving the problem using a proven approach.

We will need you to sign a Contributor Assignment Agreement (CAA) and email it to cla@hdoc.io so that this can be merged into our repository. This is unfortunately necessary for legal reasons. I have attached the CAA to this comment.

hdoc-individual-caa.pdf

no92 added 2 commits October 1, 2022 19:04
Some Linux distributions ship the clang libraries as `clang-cpp`, in my
limited testing for example Fedora and Arch Linux. To enable building
hdoc from source there, defaulting to `clang-cpp` and falling back to
the old method looks reasonable. A quick search revealed that `mesa`
uses the same approach.
As xxd uses the path name to name the variable, and the source hardcodes
the assumption that the build dir is exactly one level below the project
root, having that at a different place breaks the build. To circumvent
this issue, the variable name is passed as an extra arg to the
generator.
@no92 no92 force-pushed the compilation-fixes branch from eb536e5 to b78cd77 Compare October 1, 2022 17:07
@no92
Copy link
Author

no92 commented Oct 1, 2022

I've successfully tested building hdoc in a custom container runtime with a fairly minimal Debian-based rootfs. For my purposes, this PR is complete.

@hdoc
Copy link
Owner

hdoc commented Oct 1, 2022

We have received your CAA and we will merge your changes in with the next release of hdoc. Thank you for your contribution!

@hdoc hdoc closed this Oct 1, 2022
@eli-schwartz
Copy link

eli-schwartz commented Oct 2, 2022

It's surprising to hear that argparse doesn't compile on Arch, it must be some kind of distro-specific packaging anomaly.

No, it's a bug in argparse, which argparse fixed (in p-ranav/argparse@95d4850). Specifically, see https://gcc.gnu.org/gcc-12/porting_to.html#header-dep-changes

Some C++ Standard Library headers have been changed to no longer include other headers that were being used internally by the library. As such, C++ programs that used standard library components without including the right headers will no longer compile.

The following headers are used less widely in libstdc++ and may need to be included explicitly when compiled with GCC 12:

argparse used the code in <utility>, but didn't include it, and for a while this worked due to implementation details of other stdlib headers. Routine refactoring in GCC caused that to no longer work, and many projects adapted to this or similar changes.

@eli-schwartz
Copy link

Aside: I notice you include a bunch of subprojects used via subproject() and with the source code directly checked into git.

  • I think you should consider using Meson's wrapdb for this: https://wrapdb.mesonbuild.com. This would allow you to avoid checking 15k+ line files into git, and having that appear in commit difflogs too, and would simply download on demand. (meson dist --include-subprojects will distribute those subprojects in release tarballs.) The only one which isn't available as a wrap is cmark-gfm. Updating these would be as simple as updating a very small .ini file in git.
  • You can use dependency(), generally, and support preinstalled versions of these dependencies which people have installed in their environments. This may even be a slightly updated version that has fixes for newer versions of GCC. ;) You can still force only internally vendored copies to be used, simply invoke meson setup builddir --wrap-mode=forcefallback. You can even set wrap_mode in default_options, which would still allow users to set a non-default setup flag if they really want to use their own copies of these dependencies.

@hdoc
Copy link
Owner

hdoc commented Oct 2, 2022

@eli-schwartz Thank you for the detailed explanation of the root cause for this compile issue, now this makes sense.

Likewise thank you for your comment on our vendored dependency setup and how we can improve it. We've been meaning to contribute back to meson with our build setups for a while but never got around to it. Back when we first started hdoc the wrapdb was very small which led us to use our vendored setup. Now that it's grown it seems like a good time to switch.

On a side note I appreciate you chiming in every once in a while with your findings. This has happened more than once and it is always pleasant to see you share your knowledge with us :)

@eli-schwartz
Copy link

No problem. :) Always happy to help.

Any wrapdb contributions you have to make (such as adding cmark-gfm?) are more than welcome.

@hdoc
Copy link
Owner

hdoc commented Oct 25, 2022

@no92 This PR was mistakenly missed during the 1.4.0 release cycle and wasn't included in the release. We apologize for missing it. We will prepare a release later this week which will incorporate this change along with some other changes.

@hdoc
Copy link
Owner

hdoc commented Feb 7, 2023

Hello @no92, this PR has been merged in with release 1.4.1. Thank you for your patience and for the fix. The part of the patch relating to changing the xxd semantics was not applied as our development setup has not yet packaged the version of xxd which has support for the -name flag. Once we have support for it we will merge the rest of your change in.

Thank you once again for another contribution to the project 😃

@no92
Copy link
Author

no92 commented Aug 2, 2023

Reminder as to whether the xxd commit can be merged now, as the versions supporting the -name flag are widespread now (including e.g. Debian Bookworm).

@hdoc
Copy link
Owner

hdoc commented Aug 22, 2023

@no92 Thanks for the reminder! I just applied the patch internally and verified it works. Likewise the version of xxd we're using supports the -name argument. We're targetting October for the next release of hdoc which will include these fixes. Thank you once again for this and all your other contributions. They are much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants