Simplified Bazel build by alexsomesan · Pull Request #3142 · coreos/tectonic-installer

alexsomesan · 2018-03-28T11:25:38Z

This change proposed a simplified build flow based on the current Bazel logic.
Notable changes introduced:

Build only one platform a time. This implies releasing distinct tarballs for distinct architectures.
Use Bazel datafiles syntax instead of genrule with shell-script to copy static resources.
Remove the generated examples and replace with static cluster yaml examples.
Remove some unnecessary rules and dependencies
The release version is sourced from the version.bzl file which can be manipulated by RA.

Outstanding issues to be resolved:

drop the terraform binary next to the installer in the local dev build (the tarball already has it).
adapt documentation to explain the recommended developer workflow.

coreosbot · 2018-03-28T11:25:39Z

Can one of the admins verify this patch?

enxebre · 2018-04-03T14:52:52Z

BUILD.bazel

-load("@bazel_tools//tools/build_defs/pkg:pkg.bzl", "pkg_tar")
+template_files = glob([
+    "modules/**/*",
+    "platforms/**/*",


platforms folder does not exist anymore

Cool. I'm going to remove it here too. Thanks!

enxebre · 2018-04-03T14:53:03Z

WORKSPACE

+    commit = "361556b0d27318d1b8fa42c91a4baa4ab5ea1c58"
 )

+# http_archive(


why is this commented out?

It's mentioned in the comment above. It's because we need a change from bazel-contrib/rules_go#1393 which is not yet released. Once a release is cut, we can revert back to http_archive retrieval. That's why I kept it commented.

Once a release is cut, we can revert back to http_archive retrieval.

I've filed openshift/installer#28 with a stab at this.

enxebre · 2018-04-03T15:30:44Z

BUILD.bazel

-        "//:linux": ["@terraform_runtime_linux//:terraform"],
-        "//:darwin": ["@terraform_runtime_darwin//:terraform"],
-    }),
-    outs = ["bin/terraform"],


terraform_fmt currently relies on this output so would need to be updated

ok, I'll put it back then 👍

enxebre · 2018-04-10T08:00:29Z

BUILD.bazel

-)
+template_files = glob([
+    "modules/**/*",
+    "platforms/**/*",


we can remove platforms

enxebre · 2018-04-10T08:01:31Z

examples/BUILD.bazel

    visibility = ["//visibility:public"],
 )
+
+exports_files(example_cli_configs)


missing line

enxebre · 2018-04-10T12:14:39Z

ok to test

alexsomesan · 2018-04-10T12:39:25Z

retest this please

enxebre · 2018-04-10T12:40:57Z

When running in mac ./tectonic-dev/installer/tectonic is dropped with no execution perms 644, shouldn't it be 555?
Also could we update the tarball name and the export binary path here https://github.com/coreos/tectonic-installer/blob/master/README.md#hacking?

squat

How does this PR change the tar ball layout? Can you update the Bazel README to reflect this? Also, one of @enxebre’s comments regarding globbing “platforms” should still be addressed I think

alexsomesan · 2018-04-17T20:58:06Z

@squat Got around to amending the README. PTAL.
Also, smokes should be green now. I'll squash before merging.

alexsomesan · 2018-04-17T21:52:37Z

Good to review. PTAL

enxebre · 2018-04-18T13:24:22Z

lgtm, we probably want to squash the commits

* only build one platform at a time * copy templates + resources as runtime files on the cli target * remove unused rules and dependencies * source version for tarball from versions.bzl * version the tarball

squat

Looks good! One more piece of clean up we need to do is to remove the hand-tailored gazelle generated Bazel files in vendor. These had to be modified by hand to support declarative cross-compilation in Bazel but since we are no longer doing this and instead the tarballs only have binaries for a single platform, we can remove those changes. As it stands they will NOT be regenerated by gazelle because of the special directive in the files. We simple need to find ./installer/vendor/ -name *.BAZEL | xargs rm or the like and then re-run gazelle to regenerate. We can do this in a follow up

squat · 2018-04-23T11:41:24Z

We need to ensure we coordinate with RA team so they are aware of the new release process requiring one tarball per platform per release

alexsomesan · 2018-04-23T11:57:44Z

@squat I agree with both points!
Would you say INST-922 would be good opportunity to handle the vendor build files? I'm still hoping we can schedule that one soon, now that the installer backend code is gone too.

On the topic of integration with RA, I'd say the release process as we know it no longer applies at this time. We definitely have to convey these changes to RA nonetheless. Sudha wants to meet this week to discuss the path forward, so we'll touch on these changes and others within the installer.

Catching up with e22e31f (Simplified Bazel build, 2018-04-23, coreos/tectonic-installer#3142). Running 'bazel build tarball' on master actually gets you a tectonic-dev.tar.gz with a root tectonic-dev directory, but I've left that alone for now (maybe it depends on environment variables like cutting releases?). The tarball I built also lacks the darwin/linux subdirectories: $ tree tectonic-dev/installer/ tectonic-dev/installer/ ├── tectonic └── terraform But by stopping at the 'installer' directory I don't have to worry about figuring out how cross-platform builds work now ;).

alexsomesan requested review from enxebre, mxinden, spangenberg and squat March 28, 2018 11:25

enxebre reviewed Apr 3, 2018

View reviewed changes

alexsomesan force-pushed the bazel-streamline branch from c979919 to 4ec8f08 Compare April 6, 2018 15:16

enxebre reviewed Apr 10, 2018

View reviewed changes

BUILD.bazel Outdated

)

template_files = glob([

"modules/**/*",

"platforms/**/*",

Copy link

Contributor

enxebre Apr 10, 2018

Choose a reason for hiding this comment

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

we can remove platforms

enxebre reviewed Apr 10, 2018

View reviewed changes

enxebre previously approved these changes Apr 10, 2018

View reviewed changes

alexsomesan dismissed enxebre’s stale review via acb3ae2 April 10, 2018 10:31

alexsomesan force-pushed the bazel-streamline branch from acb3ae2 to 70b7eaa Compare April 10, 2018 10:36

enxebre added platform/aws run-smoke-tests labels Apr 10, 2018

enxebre previously approved these changes Apr 10, 2018

View reviewed changes

alexsomesan dismissed enxebre’s stale review via 121e07b April 10, 2018 12:31

alexsomesan force-pushed the bazel-streamline branch from 121e07b to b9093e0 Compare April 10, 2018 12:44

squat suggested changes Apr 11, 2018

View reviewed changes

alexsomesan force-pushed the bazel-streamline branch 2 times, most recently from f264b94 to 0f7b2c4 Compare April 17, 2018 18:04

enxebre previously approved these changes Apr 18, 2018

View reviewed changes

spangenberg previously approved these changes Apr 18, 2018

View reviewed changes

Bazel build simplifications

282a3fb

* only build one platform at a time * copy templates + resources as runtime files on the cli target * remove unused rules and dependencies * source version for tarball from versions.bzl * version the tarball

alexsomesan dismissed stale reviews from spangenberg and enxebre via 282a3fb April 18, 2018 19:04

alexsomesan force-pushed the bazel-streamline branch from 63ddcbe to 282a3fb Compare April 18, 2018 19:04

Fix the Grafitti helper to support new step separation

15d15e6

alexsomesan force-pushed the bazel-streamline branch from 78a8557 to 15d15e6 Compare April 19, 2018 13:47

squat approved these changes Apr 23, 2018

View reviewed changes

enxebre approved these changes Apr 23, 2018

View reviewed changes

alexsomesan merged commit e22e31f into coreos:master Apr 23, 2018

alexsomesan deleted the bazel-streamline branch April 23, 2018 12:50

wking mentioned this pull request Jul 7, 2018

Documentation/dev/build: Stale tree (references tectonic-installer/) openshift/installer#33

Closed

wking mentioned this pull request Jul 13, 2018

Documentation/dev/build: "tectonic-installer" -> "installer" openshift/installer#53

Merged

Conversation

alexsomesan commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coreosbot commented Mar 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enxebre commented Apr 10, 2018

Uh oh!

alexsomesan commented Apr 10, 2018

Uh oh!

enxebre commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

alexsomesan commented Apr 17, 2018

Uh oh!

alexsomesan commented Apr 17, 2018

Uh oh!

enxebre commented Apr 18, 2018

Uh oh!

squat left a comment

Choose a reason for hiding this comment

Uh oh!

squat commented Apr 23, 2018

Uh oh!

alexsomesan commented Apr 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alexsomesan commented Mar 28, 2018 •

edited

Loading

enxebre commented Apr 10, 2018 •

edited

Loading