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

hermetic builds #1155

Closed
cc10512 opened this issue Oct 15, 2024 · 4 comments · Fixed by #1160
Closed

hermetic builds #1155

cc10512 opened this issue Oct 15, 2024 · 4 comments · Fixed by #1160

Comments

@cc10512
Copy link
Contributor

cc10512 commented Oct 15, 2024

Describe the bug
Hermetic builds are a bit more strict with includes and paths.

Additional context
I have a potential PR that consists of a couple of commits enabling such builds. Two main issues addressed in different commits:

  1. adding missing includes for features used in header files.
  2. add arguments to generative scripts that allow separating the location of the script from the location of the input. Or in other words, allow inputs to be specified explicitly on the command line. In addition a few more suggestions from static checking Python tools.

Please let me know it this is of interest.

@MikePopoloski
Copy link
Owner

Yes, I would accept a PR for this, though I can't guarantee it will remain buildable if there's no CI job ensuring that the hermetic build continues to work.

@cc10512
Copy link
Contributor Author

cc10512 commented Oct 18, 2024

Thanks for your reply. You make a good point of maintenance.

I believe we have two options:

  1. add a bazel BUILD file that would do CI for this. Not ideal to have two parallel systems, but not a huge maintenance effort once we pass the initial effort (and yes, we could contribute that initial work).
  2. add a set of stricter compile options. The current tree builds with:
  add_compile_options(
      -Wall
      -Wextra
      -pedantic
      -Werror
      -Wno-ctad-maybe-unsupported
      -Wno-ignored-qualifiers
      -Wno-non-virtual-dtor
      -Wno-pessimizing-move
      -Wno-reorder-ctor
      -Wno-self-assign-overloaded
      -Wno-sign-compare
      -Wno-unused-function
      -Wno-unused-lambda-capture
      -Wno-unused-parameter
      -Wno-unused-private-field
      -Wno-unused-variable
      -Wno-unused-but-set-variable
)

We can then incrementally remove all the disabled warnings and get to a clean build with the most pedantic set of warnings. This second option does not address the file locations and Python type checking. But together with the PR I suggested, gets us quite most of the way -- the C++ source won't need any additional maintenance work.

Please let me know what you think.

@MikePopoloski
Copy link
Owner

Hmm, I'm not excited about having a second parallel build system to maintain, even if you contribute the initial implementation. I don't know much about Bazel; is it unable to consume 3rd party libraries that have a cmake build system? It's also unclear to me why the pedantic set of warnings is necessary for hermetic builds.

Maybe let's just start with your PR and we can see how much pain there is for you to maintain that over time. It's not like I'm going to go out of my way to break hermetic builds for you in the future.

@cc10512
Copy link
Contributor Author

cc10512 commented Oct 20, 2024

Fair. I opened #1160.

The pedantic warnings was just a suggestion driven by the side-effects of using a different build system with stricter defaults.

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 a pull request may close this issue.

2 participants