Add --trust_install_base option #11905

meteorcloudy · 2020-08-07T15:15:19Z

This will allow Bazel client to run individually without the appended
zip archive, which is a requirement for Debian installation.

The following changes are made:

Add --trust_install_base option. When this is enabled. Bazel client
trusts the install base provided by --install_base option and skip
the step of verifying the install directory. We need it because the
Bazel client alone doesn't contain information to verify the install
directory.
Reimplement --version as an actual Bazel startup flag. The version
info is read from the zip archive, but when --trust_install_base is
enabled it should be read from the given install base.

Related: #9408
Fixes: #11842

olekw · 2020-08-14T16:25:12Z

I can confirm that this PR allows the Bazel binary to be safely stripped and will resolve #11842. I tested with a build and install in the current Debian unstable.

This will allow Bazel client to run individually without the appended zip archive, which is a requirement for Debian installation. The following changes are made: - Add --trust_install_base option. When this is enabled. Bazel client trusts the install base provided by --install_base option and skip the step of verifying the install directory. We need it because the Bazel client alone doesn't contain information to verify the install directory. - Reimplement --version as a actual Bazel startup flag. The version info is read from the zip archive, but when --trust_install_base is enabled it should be read from the given install base. Related: bazelbuild#9408 Fixes: bazelbuild#11842

michajlo · 2020-08-20T14:58:40Z

Folding --version into regular option handling is something I had wanted to do when first adding it, but I didn't want to go too deep into figuring out/defining how it would get along with the option parsing semantics and the things that can go wrong between the 3rd line of Main and options parsing. Do you have any thoughts on those?

Also, documentation will need to be updated to reflect that --version is more lenient now, and we should add at least one test to show how it interacts with other options.

meteorcloudy · 2020-08-20T15:12:19Z

Folding --version into regular option handling is something I had wanted to do when first adding it, but I didn't want to go too deep into figuring out/defining how it would get along with the option parsing semantics and the things that can go wrong between the 3rd line of Main and options parsing. Do you have any thoughts on those?

So with the implementation in this PR. If the option parsing succeeded and --version is passed, Bazel will just print the version info, ignore other flags and possible bazel command, and exits. Comparing to the current behaviour that Bazel complains --version is not a recognized flag when other flags are passed, I think it is better.

Also, documentation will need to be updated to reflect that --version is more lenient now, and we should add at least one test to show how it interacts with other options.

Adding documentation and test sounds a good idea. Is there any original doc or test for --version available somewhere?

meteorcloudy · 2020-08-21T12:47:08Z

Test and documentation added, please take another look ;)

philwo · 2020-08-21T13:06:14Z

LGTM except for the build failures ;)

meteorcloudy · 2020-08-21T13:08:08Z

Oops, forgot to run git add 😅

Thanks for the fast review!

src/main/cpp/blaze.cc

src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java

meteorcloudy · 2020-08-21T14:14:07Z

@michajlo Thanks for the review, please take another look ;)

michajlo · 2020-08-21T14:35:56Z

site/docs/user-manual.html

  <code>bazel version --gnu_format</code>, except without the side-effect of potentially starting
  a Bazel server or unpacking the server archive. <code>bazel --version</code> can be run from
-  anywhere - it does not require a workspace directory.
+  anywhere - it does not require a workspace directory. When <code>--version</code> is passed, all
+  other flags and Bazel command will be ignored.


i wonder if people will nitpick on this... but all other flags aren't completely ignored... they're parsed (i believe including rc files) then dropped.

actually, this makes me think, should we skip rc parsing if --version is specified, so that a bad rc file doesn't mess things up? i also wonder if we can disallow --version from rc files, but that's probably not worth it...

actually, this makes me think, should we skip rc parsing if --version is specified

Sounds like a good idea, but would implement in a different PR.

michajlo · 2020-08-21T14:36:38Z

src/main/cpp/archive_utils.h

@@ -20,6 +20,11 @@

 namespace blaze {

+void DetermineArchiveContentsFromInstallBase(


document the intended use? (when install base is considered trusted, right?)

michajlo · 2020-08-21T14:42:51Z

src/main/cpp/blaze.cc

@@ -1617,11 +1631,21 @@ int Main(int argc, const char *const *argv, WorkspaceLayout *workspace_layout,
  StartupOptions *startup_options = option_processor->GetParsedStartupOptions();
  startup_options->MaybeLogStartupOptionWarnings();

+  if (startup_options->trust_install_base && startup_options->install_base.empty()) {


This is making me realize, the semantics of --version differ now based on if --trust_install_base is specified. With --trust_install_base we print the version of the server, without --trust_install_base we print the version of the client. These could differ. This should definitely be documented, or (probably overkill) we could have separate flags...

These could differ.

How?

Client and server versions fall out of sync. Could be user points client at diff install base, or that some update doesn't go as expected.

Client and server versions fall out of sync. Could be user points client at diff install base, or that some update doesn't go as expected.

FWIW, this would be a highly improbable situation in the Debian package. Certainly possible, but someone would have to purposely hack some things that they shouldn't be touching unless they're a Bazel developer or Debian package manager. ;)

This would be for uses outside of debian as well.

Well of course! I'm just commenting within the scope of my expertise. 🙂 Additional uses are likely in the future but the official Debian package is presently the only user of the trust-install-base option.

meisterT · 2020-08-31T10:56:20Z

site/docs/user-manual.html

  <code>bazel version --gnu_format</code>, except without the side-effect of potentially starting
  a Bazel server or unpacking the server archive. <code>bazel --version</code> can be run from
-  anywhere - it does not require a workspace directory.
+  anywhere - it does not require a workspace directory. When <code>--version</code> is passed, all
+  other flags and Bazel command will be ignored.


actually, this makes me think, should we skip rc parsing if --version is specified

Sounds like a good idea, but would implement in a different PR.

meisterT · 2020-08-31T11:00:14Z

src/main/cpp/blaze.cc

@@ -1007,6 +1007,11 @@ static DurationMillis ExtractData(const string &self_path,
          << "install base directory '" << install_base
          << "' could not be created. It exists but is not a directory.";
    }
+
+    if (startup_options.trust_install_base) {


I am a little surprised to see this down here.

What's the expected behavior of this option when no install base exists? I would not assume that it should create one (but it does above).

mcarpenter · 2020-10-06T19:10:16Z

Meanwhile... workaround for debian/ubuntu users whose bazel gets zapped by the prelinker:

# echo '-b /usr/bin/bazel-real' >> /etc/prelink.conf

(prelink(8) is invoked from /etc/cron.daily/prelink on Ubuntu 20.04).

meisterT · 2020-10-15T06:41:14Z

Yun, what's the state of this PR?

meteorcloudy · 2020-10-15T08:13:00Z

Sorry, I haven't found time for this recently. But it's on my to-do list. Will take on this ASAP!

aiuto · 2021-10-07T21:47:07Z

Friendly ping. If this will never merge, can you close it. That way we will stop seeing it in bug triage.

meteorcloudy · 2021-10-20T09:37:07Z

Sorry, about that. I'll close this for now until I have time to work on it again.

googlebot added the cla: yes label Aug 7, 2020

meteorcloudy force-pushed the allow_bazel_client_as_bazel_binary branch from a449315 to 5685c4c Compare August 10, 2020 15:42

meteorcloudy force-pushed the allow_bazel_client_as_bazel_binary branch from 5685c4c to 18c42b1 Compare August 20, 2020 13:32

meteorcloudy added 2 commits August 20, 2020 16:28

Add test for running bazel client

de33eb4

meteorcloudy force-pushed the allow_bazel_client_as_bazel_binary branch from 18c42b1 to de33eb4 Compare August 20, 2020 14:31

meteorcloudy changed the title ~~WIP: Add --trust_install_base option~~ Add --trust_install_base option Aug 20, 2020

meteorcloudy requested review from michajlo, meisterT and philwo August 20, 2020 14:31

meteorcloudy added 2 commits August 20, 2020 17:59

Fix test on Windows

b09a948

Add test and documentation

de51634

meteorcloudy requested review from aiuto, gregestren and jin as code owners August 21, 2020 12:45

Remove blank line

a0d083d

Add missing test file

d3d38bb

michajlo requested changes Aug 21, 2020

View reviewed changes

Address reviewer comments

ece6670

More fix on doc

92a940a

aiuto requested a review from WarkahScott August 21, 2020 14:19

Fix build error and warning on Linux

38fb2c0

aiuto removed their request for review August 21, 2020 14:21

michajlo reviewed Aug 21, 2020

View reviewed changes

meisterT reviewed Aug 31, 2020

View reviewed changes

lberki added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Oct 21, 2020

jin assigned meteorcloudy Oct 26, 2020

mr-c mentioned this pull request Nov 19, 2020

Allow safe stripping of Bazel binary #11842

Open

meteorcloudy closed this Oct 20, 2021

meteorcloudy mentioned this pull request Dec 14, 2021

deb with pre-extracted bazel #4492

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --trust_install_base option #11905

Add --trust_install_base option #11905

meteorcloudy commented Aug 7, 2020 •

edited

Loading

olekw commented Aug 14, 2020

michajlo commented Aug 20, 2020

meteorcloudy commented Aug 20, 2020

meteorcloudy commented Aug 21, 2020

philwo commented Aug 21, 2020

meteorcloudy commented Aug 21, 2020

meteorcloudy commented Aug 21, 2020

michajlo Aug 21, 2020

meisterT Aug 31, 2020

michajlo Aug 21, 2020

michajlo Aug 21, 2020

philwo Aug 21, 2020

michajlo Aug 21, 2020

olekw Aug 21, 2020

michajlo Aug 21, 2020

olekw Aug 21, 2020

meisterT Aug 31, 2020

meisterT Aug 31, 2020

mcarpenter commented Oct 6, 2020

meisterT commented Oct 15, 2020

meteorcloudy commented Oct 15, 2020

aiuto commented Oct 7, 2021

meteorcloudy commented Oct 20, 2021

		@@ -20,6 +20,11 @@

		namespace blaze {

		void DetermineArchiveContentsFromInstallBase(

Add --trust_install_base option #11905

Add --trust_install_base option #11905

Conversation

meteorcloudy commented Aug 7, 2020 • edited Loading

olekw commented Aug 14, 2020

michajlo commented Aug 20, 2020

meteorcloudy commented Aug 20, 2020

meteorcloudy commented Aug 21, 2020

philwo commented Aug 21, 2020

meteorcloudy commented Aug 21, 2020

meteorcloudy commented Aug 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcarpenter commented Oct 6, 2020

meisterT commented Oct 15, 2020

meteorcloudy commented Oct 15, 2020

aiuto commented Oct 7, 2021

meteorcloudy commented Oct 20, 2021

meteorcloudy commented Aug 7, 2020 •

edited

Loading