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

Revert "bazel: fix darwin build on hydra" #43479

Merged
merged 1 commit into from
Jul 13, 2018

Conversation

Profpatsch
Copy link
Member

Reverts #42832

We shouldn’t add hundreds of lines of copied and slightly changed source files to our package definitions. This needs to be reworked.

@GrahamcOfBorg GrahamcOfBorg added the 6.topic: darwin Running or building packages on Darwin label Jul 13, 2018
@Profpatsch
Copy link
Member Author

@uri-canva If you want, you can open a new pull request and I can review what parts need to be changed.

@Profpatsch Profpatsch merged commit adbd3ea into master Jul 13, 2018
@grahamc
Copy link
Member

grahamc commented Jul 13, 2018

cc @mboes

@grahamc grahamc deleted the revert-42832-bazel-hydra-darwin branch July 13, 2018 12:18
@matthewbauer
Copy link
Member

Sorry but there have been no PRs for alternatives yet. Bazel is a pretty important build tool and should run on macOS! Why it doesn’t is probably bazel’s fault and not Nixpkgs so I guess we should try to fix this there.

I had a branch that was a little cleaner but it never worked properly. By design bazel goes out of its way to ignore the user’s settings so some sort of hack is almost certainly needed (but i agree that it could definitely be made better than it was). But no one has done the work to do that yet.

@uri-canva uri-canva mentioned this pull request Jul 13, 2018
9 tasks
@uri-canva
Copy link
Contributor

By design bazel goes out of its way to ignore the user’s settings so some sort of hack is almost certainly needed

bazel has its own sandboxing and configuration mechanism. We can either make use of them, or hack the source to disable the sandboxing and pick up the configuration in the way we specify it in nix.

I went with the first approach, because I think it's cleaner, but it's possible to use the second approach by having two version of bazel: one with the hacks so that it can be used in nix, which can then be used to build one without hacks to be used outside of nix, skipping the compile.sh script.

@LnL7
Copy link
Member

LnL7 commented Jul 13, 2018

I don't really like that this was insta-merged without discussion/feedback from other members. Reverting a change that causes a regression is another story, but something like this which almost comes down to personal opinion should go through the same process as other nontrivial changes IMHO.

@Profpatsch
Copy link
Member Author

Profpatsch commented Jul 13, 2018

Not to stir up an argument here, but this goes both ways; introducing such a big change (a 300% blowup in code size after all) to a package should not be merged lightly, and wait at last a few days to see if the maintainer has any comments (the PR was merged after one day).
Especially in this case, where an update PR by the maintainer was pending at the same time.

@Profpatsch
Copy link
Member Author

I added a few comments to the original PR as requested.

@LnL7
Copy link
Member

LnL7 commented Jul 15, 2018

I'm not talking about the changes itself and mostly agree with the arguments there. I just think something like this shouldn't be merged instantly.

@uri-canva uri-canva mentioned this pull request Jul 28, 2018
9 tasks
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.

6 participants