-
Notifications
You must be signed in to change notification settings - Fork 772
{devel}[Java] Bazel 0.4.4 #4114
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
Conversation
|
This is a clean version of Bazel with the only dep being Java. It will be necessary for bazel builds (like tensorflow) |
|
|
||
| sanity_check_paths={ | ||
| 'files': ['bin/bazel'], | ||
| 'dirs': ['bin'], |
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 useless if you're already checking for files in bin/, but fine :)
|
|
||
| dependencies = [ | ||
| ('Java', '1.8.0_121'), | ||
| ] |
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.
please move this down, below source_urls
|
|
||
| sources = ['%(namelower)s-%(version)s-dist.zip'] | ||
| source_urls = ['https://github.com/bazelbuild/bazel/releases/download/%(version)s'] | ||
| cmds_map = [('.*', "JAVA_VERSION=1.8 CC=gcc CXX=g++ ./compile.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.
please leave a blank line above cmds_map, and try to avoid hardcoding the Java version; you can use %(javashortver)s instead (cfr. http://easybuild.readthedocs.io/en/latest/version-specific/easyconfig_templates.html#template-names-values-for-short-software-versions)
|
|
||
| sanity_check_paths={ | ||
| 'dirs': ['bin'], | ||
| 'files': ['bin/bazel'] |
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.
@rjeschmi nitpicking, what I was going for is:
sanity_check_paths = {
'files': ['bin/bazel'],
'dirs': [],
}i.e. , there's no point in checking for a non-empty bin dir if you're also checking for bin/bazel
Style-wise: keep the files/dirs order like we do in other easyconfigs, use spaces around =.
|
|
||
| sources = ['%(namelower)s-%(version)s-dist.zip'] | ||
| source_urls = ['https://github.com/bazelbuild/bazel/releases/download/%(version)s'] | ||
| cmds_map = [('.*', "JAVA_VERSION=1.8 CC=gcc CXX=g++ ./compile.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.
Maybe set $CC to cc and $CXX to c++ in order to match to the implicit rules that GNU make has as well? Or not use dummy as TC but set it to some GCC core version? Just a suggestion ...
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 don't even know if I really need to set anything. I'll just remove it I think.
|
|
||
| sanity_check_paths={ | ||
| 'files': ['bin/bazel'], | ||
| 'dirs': ['bin'], |
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.
Not sure what the advantage would be to check for bin/foo and bin/ separately...
|
Test report by @rjeschmi |
|
Test report by @rjeschmi |
|
Test report by @rjeschmi |
|
Test report by @rjeschmi |
|
Test report by @rjeschmi |
|
Test report by @rjeschmi |
|
Test report by @rjeschmi |
|
Test report by @rjeschmi |
|
I'm just testing the build test from singularity. Should work (based on my docker image) |
|
[rob@ottbioinfo volumes]$ singularity run -B etc/:/easybuild/etc -B export:/export easybuild.img --from-pr=4114 --upload-test-report --github-user=rjeschmi --robot I have some problem with sources (coming out of my home directory now for simplicity) I can also probably fix the etc mapping (have to write to /export for the tests in this case since /easybuild is read-only) |
|
Test report by @rjeschmi |
|
@rjeschmi having fun? You don't need to use |
|
Test report by @boegel |
|
Test report by @boegel |
|
lgtm |
|
Going in, thanks @rjeschmi! |
|
Test report by @rjeschmi |
|
Test report by @rjeschmi |
|
Test report by @rjeschmi |
Built using java 1.8.