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.

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

Warn the user about required PATH entry ordering #1933

Closed
wants to merge 2 commits into from

Conversation

petemounce
Copy link
Contributor

@petemounce petemounce commented Oct 12, 2016

As discovered within #1919, the order of entries in the PATH matters, and is hard to diagnose when it's wrong.

  • add the warning message
  • try it out on clean windows box
  • ❓ mention this in the windows.md doc? It's a pretty inscrutable environment error, and I'd assume most people installing bazel on windows will already have git, so would be affected right out of the gate.
  • bump the package version number according to chocolatey's package fix version notation

Once I've done both of those, I'll publish the package after this has been merged.

@bazel-io
Copy link
Member

Can one of the admins verify this patch?

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@petemounce
Copy link
Contributor Author

I signed it!

As discovered within bazelbuild#1919, the order of entries in the `PATH` matters, and is hard to diagnose when it's wrong.
docs: https://github.com/chocolatey/choco/wiki/CreatePackages and http://docs.nuget.org/ndocs/create-packages/dependency-versions

The docs suggest things that did not work for me. I was able to get bazel to install by first, separately, installing msys2
Copy link
Contributor

@dslomov dslomov left a comment

Choose a reason for hiding this comment

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

This is good, although it would be even more awesome if we actually detected the failuer condition and warned user (if user axtually has msys-2.0.dll in path that is not from c:\tools\msys2\usr\bin or bash.exe that is from c:\windows\ (not asking to do that in this PR, just a general idea)

@@ -76,7 +76,7 @@ Supply like `--params="/option:value ..."` ([see docs for --params](https://gith
</dependencies>-->
<dependencies>
<dependency id="jdk8" version="[8.0.102,)"/>
<dependency id="msys2" version="[20160205,)"/>
<dependency id="msys2" version="[20160719.1,)"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

20160205 should be fine no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be, but has not been published to the feed.

@petemounce
Copy link
Contributor Author

re: more awesome; yes, definitely. Thought about it. But it would require knowing and maintaining a list of apps that vendor msys2 I think? Or is there a way to detect that from PowerShell, before the zip is fetched and installed? Same for bash.exe. That also discounts manual installs of same.

@dslomov
Copy link
Contributor

dslomov commented Oct 13, 2016

I was thinking just check the output of where msys-2.0.dll and yell at user if it is not in c:\tools\msys64\usr\bin. Ditto for where bash.exe (yell if it is in c:\windows\ because the it is definitely from u-o-w. Maybe not yell very loudly since it might still be ok.

@petemounce
Copy link
Contributor Author

I updated the docs via #1938.

@abergmeier-dsfishlabs
Copy link
Contributor

abergmeier-dsfishlabs commented Jan 26, 2017

@dslomov Given the amount of time that multiple people now wasted and wasting with this - please check the found msys-2.0.dll for version or ABI compat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants