-
Notifications
You must be signed in to change notification settings - Fork 202
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
Unify Dockerfile
and configure.sh
, upgrade to LLVM 14, build LevelDB from source
#162
Conversation
652a6b6
to
602d1fd
Compare
I really like a lot of the simplifications present in this. @jallen-frb can some/all of these simplifications be folded into (e.g., by rebasing your work on top of these commits) #161? |
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.
untested ACK.
I only have one question (which isn't specific to a change included in this PR, but rather a question if we can improve a single line of code this this PR touches) made inline.
Signed-off-by: James Lovejoy <[email protected]>
Signed-off-by: James Lovejoy <[email protected]>
Signed-off-by: James Lovejoy <[email protected]>
Signed-off-by: James Lovejoy <[email protected]>
Signed-off-by: James Lovejoy <[email protected]>
07cdbc7
to
354fa31
Compare
Looks good to me. Will be testing locally shortly. |
tested ACK. This looks good and behaves well for me. My only question is about whether or not some of the proposed changes from #171 should be included here to minimize breakage potential. |
@HalosGhost I think those changes probably make most sense to be included in #170. Since the snappy linkage issue doesn't seem to occur on Ubuntu or MacOS (which are the current supported platforms). |
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.
Solid improvement and simplification to the docker instrumentation by leveraging tooling we already have. And, as a bonus, it unifies more dependencies across platforms and updates LLVM.
Does what it says on the tin. Looks good to me.
My build env is Arch Linux and snappy library package doesn't include static in opposite to ubuntu packages.
|
This PR:
Dockerfile
, usingconfigure.sh
instead.clang-tidy
. Disables the newreadability-identifier-length
check for now, as large parts of the codebase are not compliant (perhaps this should be an issue?). This is needed to supportNOLINTBEGIN
andNOLINTEND
, which will be used in a subsequent PR.configure.sh
to remove the dependency cache, which I don't think is used in our current iteration of Github actions, or when building using docker.lint.sh
regex to only include files with the*pp
extension, rather than any file ending inpp
.This probably conflicts with #161.