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
bs-platform: 7.0.1 -> 7.2.0 #82068
bs-platform: 7.0.1 -> 7.2.0 #82068
Conversation
9930462
to
7107e39
Compare
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.
- Tested on NixOS
just few questions before we go ahead with merge.
BS_RELEASE_BUILD = "true"; | ||
BS_TRAVIS_CI = "1"; |
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.
Can you elaborate on why we need this?
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 some reason (cf rescript-lang/rescript-compiler#4183) BuckleScript added yet another idiosyncrasy to its build / release process. This environment variable allows the intermediate artifacts of the stdlib to be built so that editor auto completion works, for example.
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 explanation! I would probably add some short comment about this as it's really non obvious from code itself. Thanks
cp bsconfig.json package.json $out | ||
ln -s $out/lib/bsb $out/bin/bsb | ||
ln -s $out/lib/bsc $out/bin/bsc | ||
ln -s $out/lib/bsrefmt $out/bin/bsrefmt |
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.
bsrefmt is gone?
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.
This is probably an oversight. I’ll check soon!
@turboMaCk addressed your code review. |
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.
Looks great to me!
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 this. Built on MacOS too 👍
@jonringer could I request a merge from you? |
Thanks! |
sorry, haven't had as much free time lately |
Motivation for this change
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)