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

Parse params via a built-in, and lock msys2 version #2690

Closed
wants to merge 4 commits into from
Closed

Parse params via a built-in, and lock msys2 version #2690

wants to merge 4 commits into from

Conversation

petemounce
Copy link
Contributor

@petemounce petemounce commented Mar 16, 2017

@laszlocsomor - This addresses #2449 (comment).

Note - this is not in the 0.4.5 package, since I was waiting for that release to go out prior to this.

@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.

<dependency id="jdk8" version="[8.0.102,)"/>
<dependency id="msys2" version="[20160719.1,)"/>
<dependency id="msys2" version="[20160719.1.0]"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@laszlocsomor - I'm going on trust that this is the version that you'd like to pin to. https://chocolatey.org/packages/msys2 lists the ones that are available.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to pin to an exact version, or is it enough to pin 20160719.1.x?

Copy link
Contributor Author

@petemounce petemounce Mar 16, 2017

Choose a reason for hiding this comment

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

You asked me to pin it to the precise version inside #2449 (comment) on the basis that others didn't work for bazel (at the time).

I haven't tested that assertion, and also haven't seen issues reported about bazel failures against later versions (there's 20160719.1.1, and there are also later versions in the moderation queue (this is a paged link)).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah :) I've seen failures with newer major versions (20170130 specifically), but I also haven't tested with 20160719's other minor/patch versions. The problem was binary incompatibility with msys-2.0.dll between the two major versions, but I believe they should be compatible within the same major one.

Aaand fortunately this will soon not matter because we're close to removing the msys dependency :) (we'll still need some msys installed in order to have a bash.exe to run, but bazel itself won't be tied to a given version).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, shall I simply back out this change (the version pin) as unnecessary?

Copy link
Contributor

@laszlocsomor laszlocsomor Mar 20, 2017

Choose a reason for hiding this comment

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

No, please don't. As i understand it, both 20160719.1.0 and 20160719.1.1 are good, but versions with a more recent first part (e.g. 20170130.x.y) seem to be bad. So the pin should be [20160719.1.0,20160719.1.1].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@petemounce
Copy link
Contributor Author

I signed it!

@laszlocsomor
Copy link
Contributor

Importing. Not sure why you keep getting the "cla: no" status. :( Could it be that you're using a different github account than before?

@laszlocsomor
Copy link
Contributor

...or different email?

@laszlocsomor
Copy link
Contributor

laszlocsomor commented Mar 20, 2017

I tried to figure out why you keep getting the "cla: no" label. I could verify that Improbable signed a corporate CLA, so the question is why your account isn't associated with it. Could you try finding out please who's managing Improbable's CLA group and have them add your email to it?

Also, what do you see on https://cla.developers.google.com/clas?

@petemounce
Copy link
Contributor Author

I see this:
clas-portal

I've had my improbable.io email account added to my github profile since probably late September at least. I'm definitely using the same github account as ever.

@petemounce
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@petemounce
Copy link
Contributor Author

Hurrah! Turns out I wasn't a member of the google group that we used to sign that; now I am.

@laszlocsomor
Copy link
Contributor

Hooray! Great. Thanks for following through! :) Importing now.

kchodorow pushed a commit to kchodorow/bazel that referenced this pull request Mar 21, 2017
@laszlocsomor - This addresses bazelbuild#2449 (comment).

Note - this is _not_ in the 0.4.5 package, since I was waiting for that release to go out prior to this.

Closes bazelbuild#2690.

PiperOrigin-RevId: 150745085
@bazel-io bazel-io closed this in 8bd5522 Mar 22, 2017
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

4 participants