Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added support for win_flex_bison on Windows #771
Added support for win_flex_bison on Windows #771
Changes from all commits
20ad8dc
c16b032
9532346
2ac5215
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this
win_flex_configure()
be made to return early if we're not on Windows ?There seems to be an issue with pre-fetching all the needed dependencies vs. what is then actually needed in download later in environments that separate out the fetching and building like on nixos; this is why it needs this patch:
https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/language-servers/verible/remove-unused-deps.patch
I suspect what is happening: the
win_flex_configure()
called from theWORKSPACE
unconditionally downloads the dependencies , but then on a non-Windows system, theselect()
in the{bison,flex}.bzl
realizes that it doesn't actually needs it, so it then leaves it out of the prepeared download blob.Later, after disconnect from the network, on
build
, the unconditional download attempt will fail. At least this is what I think is happening.Anyway, if here, we could just skip calling the
remote_win_flex_bison()
and register toolchain part if we're not on windows, that should probably help. Do you think you can add that @corco ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll will look at it at the end of the week
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look. It seems that this issue is part of the problem. There may be a way to fix it in either nix or bazel, but I am not comfortable with either...
I would suggest to either wait until bazel fix the issue, or simply remove downloading win_flex_bison and require the user to install it prior with chocolatey, like the use_local_flex_bison flag. I would move toward that second solution if @hzeller agrees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hzeller Have you look at my previous comment now that Christmas vacations are over?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, indeed I didn't see your previous comment.
I think removing the download and let the user
use_local_flex_bison
sounds good to me.