-
Notifications
You must be signed in to change notification settings - Fork 11
Standardize on using dune to build CodeHawk #196
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
Standardize on using dune to build CodeHawk #196
Conversation
Remove the Shake-based build (even though it's a great example!).
CodeHawk/README.md
Outdated
~/.cabal/bin/shake | ||
``` | ||
# Run with sudo if you wish to install globally | ||
bash -c "sh <(curl -fsSL https://opam.ocaml.org/install.sh)" |
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.
apt-get install opam
should work in 22.04?
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.
Better to keep the instructions as OS-neutral as possible?
CodeHawk/README.md
Outdated
### Homebrew | ||
sudo apt install --no-install-recommends software-properties-common \ | ||
build-essential unzip \ | ||
pkg-config m4 zlib1g-dev libgmp-dev bubblewrap -y |
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.
some questions/comments:
- I think
software-properties-common
was only needed to add the opam ppa. - what do we need
bubblewrap
for? - could this be missing the jdk dependency to open the
jar
summary files? (we really should replace that withzip
).
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.
Ah, could be!
Opam uses bubblewrap to sandbox the builds it runs so that buggy shell scripts do not inadvertently rm -rf ~/
The prior instructions bypass the bwrap dependency via --no-sandboxing
but that seems... not the right guidance to be giving without clear justification.
Re: jar files, one of the libraries CodeHawk depends on is camlzip
, I'm not sure if it's already used for jar files though.
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.
CodeHawk-Binary
depends on literally the jar
executable from the JDK, though it uses it to make regular zip files that are interpreted by camlzip
in this project as you mention and not any actual JRE.
CodeHawk/README.md
Outdated
/path/to/shake | ||
``` | ||
Dependencies for other OS flavors: | ||
- Arch Linux: `sudo pacman -Syu opam` |
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.
it seems like the extra deps from the other distros should also go in here? #untested
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.
Maybe we should remove the instructions for other OS flavors? I haven't tested them.
CodeHawk/README.md
Outdated
``` | ||
Dependencies for other OS flavors: | ||
- Arch Linux: `sudo pacman -Syu opam` | ||
- Fedora: `sudo yum install opam diffutils zlib-devel ocaml-lablgtk-devel -y` |
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.
chances are you don't need the gtk
package, that sounds like a GUI thing
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.
Thanks!
CodeHawk/README.md
Outdated
### Homebrew | ||
sudo apt install --no-install-recommends software-properties-common \ | ||
build-essential unzip \ | ||
pkg-config m4 zlib1g-dev libgmp-dev bubblewrap -y |
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.
CodeHawk-Binary
depends on literally the jar
executable from the JDK, though it uses it to make regular zip files that are interpreted by camlzip
in this project as you mention and not any actual JRE.
### Arch Linux | ||
opam switch create . 5.2.0 | ||
eval $(opam env) | ||
opam install dune ocamlfind zarith camlzip extlib goblint-cil.2.0.6 |
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.
What makes it necessary to pin goblint-cil
at a specific version while leaving the others at arbitrary versions?
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.
goblint-cil
has released API-incompatible changes in point version bumps... I think three times in the last two years?
CodeHawk's other dependencies have remained API-stable, so there is no particular reason to pin them.
CodeHawk/README.md
Outdated
/path/to/shake | ||
``` | ||
Dependencies for other OS flavors: | ||
- Arch Linux [untested]: `sudo pacman -Syu opam` |
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.
For completeness this should be sudo pacman -Syu base-devel git opam
Validated with
$ sudo docker run -it --rm archlinux bash
$ pacman -Syu base-devel git opam
then the rest of the instructions.
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.
Thank you for doing this testing!
CodeHawk/README.md
Outdated
``` | ||
Dependencies for other OS flavors: | ||
- Arch Linux [untested]: `sudo pacman -Syu opam` | ||
- Fedora [untested]: `sudo yum install opam diffutils zlib-devel -y` |
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.
For completeness this should be sudo yum install awk diffutils git gmp-devel opam perl-ExtUtils-MakeMaker perl-FindBin perl-Pod-Html zlib-devel -y
Validated with
$ sudo docker run -it --rm fedora bash
$ yum install awk diffutils git gmp-devel opam perl-ExtUtils-MakeMaker perl-FindBin perl-Pod-Html zlib-devel -y
then the rest of the instructions.
CodeHawk/README.md
Outdated
opam install ocamlfind zarith camlzip extlib lablgtk lablgtk-extras | ||
/path/to/shake | ||
``` | ||
Dependencies for other OS flavors: |
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.
Rather than organizing the page as
- Dependencies (assuming Debian/Ubuntu)
- Install instructions
- Dependencies (other distros)
can the page use collapsed sections to group all the operating system / distro instructions together without taking up too much space? That doesn't favor any operating system or distribution in particular.
CodeHawk/README.md
Outdated
2. Install the shake build system | ||
3. Install ocaml | ||
4. Build CodeHawk | ||
sudo apt install --no-install-recommends \ |
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.
On the latest Debian there is no openjdk-21-jdk
, and this additionally needs ca-certificates
and git
.
Validated with
$ sudo docker run -it --privileged --rm debian bash
$ apt update -y
$ apt install --no-install-recommends build-essential opam unzip ca-certificates git pkg-config m4 zlib1g-dev libgmp-dev bubblewrap -y
then the rest of the instructions.
--privileged
was necessary for bubblewrap
to work.
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 guess the previous instructions had been giving instructions for users who presumably already have git installed, rather than brave souls running commands in fresh containers :-) But on balance I think you are right, it's better to err on the side of giving reproducible instructions rather than minimal instructions.
Re openjdk-21-jdk
you are indeed correct, it seems to be an Ubuntu-ism. I'll switch the instructions to use default-jdk
which works on both Ubuntu and Debian.
CodeHawk/README.md
Outdated
2. Install the shake build system | ||
3. Install ocaml | ||
4. Build CodeHawk | ||
sudo apt install --no-install-recommends \ |
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.
On the latest Ubuntu this also should include git
, for completeness.
Validated with
$ sudo docker run -it --privileged --rm ubuntu bash
$ apt update -y
$ apt install --no-install-recommends build-essential git opam unzip openjdk-21-jdk pkg-config m4 zlib1g-dev libgmp-dev bubblewrap -y
then the rest of the instructions.
--privileged
was necessary for bubblewrap
to work.
a882fc5
into
static-analysis-engineering:master
Also remove the Shake-based build (even though it's a great example!).