Skip to content
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.

Sign up for GitHub

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

Jump to bottom

improve Bazel easyblock: add support for running tests, enable static linking, use build dir rather than tmpdir, verbose output #2285

Merged
merged 6 commits into from
Jan 20, 2021
Merged

improve Bazel easyblock: add support for running tests, enable static linking, use build dir rather than tmpdir, verbose output #2285

merged 6 commits into from
Jan 20, 2021

Conversation

Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Dec 16, 2020 •
edited
Loading

(created using eb --new-pr)

Supersedes, closes #2272

This improves the Bazel EasyBlock in various areas:

  • Constrain build to build dir, avoid Bazel using the temp dir
  • Embed the Bazel version into the resulting binary to avoid warnings like "Failed to detect Bazel version, assuming x.y" on use
  • VERBOSE build by default
  • Static build as in ENV not propagated to process-wrapper bazelbuild/bazel#4137 they said this is the way
  • Support for running tests -> This reproduces the failure caused by non-static builds

Can be tested with easybuilders/easybuild-easyconfigs#11894

@Flamefire Flamefire mentioned this pull request Dec 16, 2020
Merged
1 task
@boegelbot

This comment has been minimized.

Sign in to view
@Flamefire
Copy link
Contributor Author

@boegel Did someone broke the CI? I didn't touch mothur

@Flamefire Flamefire force-pushed the 20201216181038_new_pr_UPyhosTAbg branch from 3af6dac to 456f549 Compare December 17, 2020 08:51
@boegelbot

This comment has been minimized.

Sign in to view
@boegelbot

This comment has been minimized.

Sign in to view
@Flamefire
Copy link
Contributor Author

PR fixing only the test issues: #2287

@Flamefire Flamefire force-pushed the 20201216181038_new_pr_UPyhosTAbg branch 2 times, most recently from 754bf32 to 8fdcf25 Compare December 21, 2020 08:32
@Flamefire Flamefire mentioned this pull request Dec 22, 2020
Merged
6 tasks
lexming
lexming reviewed Dec 22, 2020
View reviewed changes
easybuild/easyblocks/b/bazel.py Outdated Show resolved Hide resolved
easybuild/easyblocks/b/bazel.py Show resolved Hide resolved
@boegel boegel added this to the next release (4.3.3?) milestone Dec 31, 2020
@boegel boegel changed the title Improve Bazel EasyBlock improve Bazel easyblock: add support for running tests, enabl static linking, use build dir rather than tmpdir, verbose output Dec 31, 2020
@boegel boegel mentioned this pull request Dec 31, 2020
Closed
@Flamefire
Copy link
Contributor Author

Test report by @Flamefire

Overview of tested easyconfigs (in order)

  • SUCCESS Bazel-1.1.0-GCCcore-8.3.0.eb
  • SUCCESS Bazel-2.0.0-GCCcore-8.3.0.eb
  • SUCCESS Bazel-3.4.1-GCCcore-8.3.0.eb
  • SUCCESS Bazel-0.20.0-GCCcore-8.2.0.eb
  • SUCCESS Bazel-0.25.2-GCCcore-8.2.0.eb
  • SUCCESS Bazel-0.26.1-GCCcore-8.2.0.eb
  • SUCCESS Bazel-0.26.1-GCCcore-8.3.0.eb
  • SUCCESS Bazel-0.29.1-GCCcore-8.2.0.eb
  • SUCCESS Bazel-0.29.1-GCCcore-8.3.0.eb
  • SUCCESS Zip-3.0-GCCcore-9.3.0.eb
  • SUCCESS Bazel-3.6.0-GCCcore-9.3.0.eb

Build succeeded for 11 out of 11 (10 easyconfigs in total)
taurusml6 - Linux RHEL 7.6, POWER, 8335-GTX, Python 2.7.5
See https://gist.github.com/c6ff6030eee262a34495a3ef8ac6f2e4 for a full test report.

@Flamefire
Copy link
Contributor Author

@boegel ping, this is required for TF 2.4 (tests at least). I believe all requests have been met and I tested with 10 existing Bazel ECs

@lexming
Copy link
Contributor

lexming commented Jan 17, 2021

Test report by @lexming

Overview of tested easyconfigs (in order)

Build succeeded for 2 out of 3 (3 easyconfigs in total)
node381.hydra.os - Linux centos linux 7.9.2009, x86_64, Intel(R) Xeon(R) Gold 6148 CPU @ 2.40GHz, Python 2.7.5
See https://gist.github.com/330b534089961de8e9dbf3b5e9b81105 for a full test report.

lexming
lexming reviewed Jan 18, 2021
View reviewed changes
Copy link
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Flamefire tests with old easyconfigs fail for unrelated reasons to your PR. However, I think that we could also fix those in here. The reason of the failed tests is that the bootstrap script (scripts/bootstrap/compile.sh) extracts .jar files with unzip and that fails for older versions with the error

error: invalid zip file with overlapped components (possible zip bomb)

The solution is to extract those .jar files with the jar command. I made a patch that fixes this issue and that can be applied to all current easyconfigs of Bazel in EB. See lexming/easybuild-easyconfigs@b22b750

Since the patch applies to all versions, could you update this PR to apply those changes to the bootstrap script?

@Flamefire
Copy link
Contributor Author

Well, I'd rather not. First because the affected ECs are old and going forward this fix will become irrelevant (GCC 7.3 is 2018 or 2017? So at least 3 generations behind) and second as it works for "most" versions and is fixed in more recent versions I'd say this is a plain bug in a few versions and hence exactly what patches are for. So I'd say it is better to update older ECs (if required) to include your patch.

It is also questionable if those versions are even still required. From your report I see 0.16 failing but 0.18 working and both in the same TC generation, so likely the latter can already be used without any problems.

@Flamefire Flamefire mentioned this pull request Jan 20, 2021
Closed
@boegel boegel changed the title improve Bazel easyblock: add support for running tests, enabl static linking, use build dir rather than tmpdir, verbose output improve Bazel easyblock: add support for running tests, enable static linking, use build dir rather than tmpdir, verbose output Jan 20, 2021
boegel
boegel approved these changes Jan 20, 2021
View reviewed changes
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@boegel
Copy link
Member

boegel commented Jan 20, 2021

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS Bazel-0.18.0-GCCcore-7.3.0.eb
  • SUCCESS Bazel-0.20.0-GCCcore-7.3.0.eb
  • SUCCESS Bazel-0.20.0-GCCcore-8.2.0.eb
  • SUCCESS Bazel-0.25.2-GCCcore-8.2.0.eb
  • SUCCESS Bazel-0.26.1-GCCcore-8.2.0.eb
  • SUCCESS Bazel-0.26.1-GCCcore-8.3.0.eb
  • SUCCESS Bazel-0.29.1-GCCcore-8.2.0.eb
  • SUCCESS Bazel-0.29.1-GCCcore-8.3.0.eb
  • SUCCESS Bazel-0.7.0-GCCcore-6.4.0.eb
  • SUCCESS Bazel-1.1.0-GCCcore-8.3.0.eb
  • SUCCESS Bazel-2.0.0-GCCcore-8.3.0.eb
  • SUCCESS Bazel-3.4.1-GCCcore-8.3.0.eb
  • SUCCESS Bazel-3.6.0-GCCcore-9.3.0.eb

Build succeeded for 13 out of 13 (13 easyconfigs in total)
node3124.skitty.os - Linux centos linux 7.9.2009, x86_64, Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz, Python 3.6.8
See https://gist.github.com/d618848c9bc03f1af6a296903e804143 for a full test report.

@boegel
Copy link
Member

boegel commented Jan 20, 2021

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS Bazel-0.18.0-GCCcore-7.3.0.eb
  • SUCCESS Bazel-0.20.0-GCCcore-7.3.0.eb
  • SUCCESS Bazel-0.20.0-GCCcore-8.2.0.eb
  • SUCCESS Bazel-0.25.2-GCCcore-8.2.0.eb
  • SUCCESS Bazel-0.26.1-GCCcore-8.2.0.eb
  • SUCCESS Bazel-0.26.1-GCCcore-8.3.0.eb
  • SUCCESS Bazel-0.29.1-GCCcore-8.2.0.eb
  • SUCCESS Bazel-0.29.1-GCCcore-8.3.0.eb
  • SUCCESS Bazel-0.7.0-GCCcore-6.4.0.eb
  • SUCCESS Bazel-1.1.0-GCCcore-8.3.0.eb
  • SUCCESS Bazel-2.0.0-GCCcore-8.3.0.eb
  • SUCCESS Bazel-3.4.1-GCCcore-8.3.0.eb
  • SUCCESS Bazel-3.6.0-GCCcore-9.3.0.eb

Build succeeded for 13 out of 13 (13 easyconfigs in total)
node2658.swalot.os - Linux centos linux 7.9.2009, x86_64, Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz (haswell), Python 3.6.8
See https://gist.github.com/843f492e374a5b930ac809a71377951d for a full test report.

@boegel boegel merged commit 4190e64 into easybuilders:develop Jan 20, 2021
@Flamefire Flamefire deleted the 20201216181038_new_pr_UPyhosTAbg branch January 21, 2021 07:44
This was referenced Feb 22, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lexming lexming lexming left review comments

@boegel boegel boegel approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
4.3.3
Development

Successfully merging this pull request may close these issues.
4 participants
@Flamefire @boegelbot @lexming @boegel