-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add wyrm #76
Add wyrm #76
Conversation
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 double check the tarball generated has the layout and contents you're happy with :) but LGTM! Next step would be to chuck it in the yaml file in the compiler workflows (and make update-compilers
or whatever there)
@@ -44,7 +44,8 @@ RUN apt update -y -q && apt upgrade -y -q && apt update -y -q && \ | |||
re2c \ | |||
perl \ | |||
cpanminus \ | |||
openssh-client | |||
openssh-client \ |
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.
Just for context: I've been trying to avoid adding more and more stuff to this Dockerfile; though this OK for this size of project. The newer "misc" stuff all has its own Docker file and build scripts etc separately so we don't just keep making this one bigger and bigger and we never quite know why we have all these dependencies.
VERSION=trunk-$(date +%Y%m%d) | ||
BRANCH=main | ||
else | ||
BRANCH=V${VERSION} |
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.
Doesn't look like yuo've had any releases yet (which is fine!) -- most github stuff uses a lower-case v
prefix just as an observation.
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 for the observation! I copied this from the movfuscator build script which I copied from something else I think, the branch isn't entered on either currently :)
misc/build-wyrm-transpiler.sh
Outdated
|
||
mkdir -p "${PREFIX}" | ||
cd .. | ||
mv build "${PREFIX}/build" |
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.
Do we want the tarball to contain this build
subdirectory? I think this will keep that dir in $PREFIX and I'd expect us not to want the build/
directory there and also all the contents (like all the intermediate CMake files etc too? though I imagine they're not too big?) In other cmake-based setups we use a ninja install
step or whatever, but it does rely on the upstream having set up installation properly :)
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 a ninja install is a good idea. I looked at the contents of the tarball briefly and figured "well I see the .so somewhere so lgtm!" but this is a good point
Build script for a research project I worked on a while back