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

Set HOMEBREW_DEVELOPER automatically #881

Merged
merged 13 commits into from Sep 9, 2016

Conversation

Projects
None yet
10 participants
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Sep 5, 2016

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

Rather than making people set it manually or assuming that certain people are/aren't developers: let's assume anyone who runs anything in dev-cmd or formulae-editing things like brew create, brew edit etc. are HOMEBREW_DEVELOPERs.

What other commands should we add in here? audit? test? Basically want to get anything that would be only run by formulae editors.

TODO:

  • Fix up brew man so that it documents these commands but sticks them in a DEVELOPER COMMANDS section.

CC @sigmavirus24 @jasonkarns @anthonygreen @maxnordlund @ilovezfs @DomT4 for thoughts and review.

Fixes #737

@MikeMcQuaid MikeMcQuaid referenced this pull request Sep 5, 2016

Closed

Brew auto-changes branch when developing formula #737

3 of 3 tasks complete
if [[ -z "$HOMEBREW_DEVELOPER" ]]
then
git config --file="$HOMEBREW_GIT_CONFIG_FILE" --replace-all homebrew.developermode true
echo "Homebrew developer mode: enabled." >&2

This comment has been minimized.

@maxnordlund

maxnordlund Sep 5, 2016

Contributor

Shouldn't these be in the same order as above in the ruby version?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 7, 2016

Member

Updated for consistency, thanks.

@@ -21,11 +21,7 @@ def command
def internal_command_path(cmd)

This comment has been minimized.

@maxnordlund

maxnordlund Sep 5, 2016

Contributor

I may be wrong here, but this seem to do the same as command_path in help.rb now, but much nicer. If it does, we can just use this one instead, but I'm not sure where it should live.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 7, 2016

Member

Good point, done.

@@ -72,11 +72,11 @@ def help(cmd = nil, flags = {})
def command_path(cmd)

This comment has been minimized.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 7, 2016

Member

Good point, done.

@maxnordlund

This comment has been minimized.

Copy link
Contributor

maxnordlund commented Sep 5, 2016

I love it, this would make me avoid much confusion to why things don't seem to work when it's because I forgot HOMEBREW_DEVELOPER=1.

if [[ -z "$HOMEBREW_DEVELOPER" ]]
then
export HOMEBREW_GIT_CONFIG_FILE="$HOMEBREW_REPOSITORY/.git/config"
HOMEBREW_GIT_CONFIG_DEVELOPERMODE="$(git config --file="$HOMEBREW_GIT_CONFIG_FILE" --get homebrew.developermode)"

This comment has been minimized.

@chdiza

chdiza Sep 5, 2016

Contributor

Isn't this going to add overhead to every brew incantation? I'm all for making the dev-cmd's available to anyone, but I'm not sure I see why there needs to be this git config file trick. I can't think of why someone would want HOMEBREW_DEVELOPER _un_set but have something in the repo's gitconfig saying "I'm a developer".

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 7, 2016

Member

The overhead is tiny (0.004s on my machine) and already exists for analytics checks.

I can't think of why someone would want HOMEBREW_DEVELOPER unset but have something in the repo's gitconfig saying "I'm a developer".

That's kinda the point of this PR 😉

This comment has been minimized.

@chdiza

chdiza Sep 7, 2016

Contributor

That's kinda the point of this PR

And that's what I don't think I understand. I thought the point was to stop requiring HOMEBREW_DEVELOPER be set in order to use the dev-cmds (et al). Why not just remove the "don't do anything unless HOMEBREW_DEVELOPER is set" guard on the dev-cmds (et al), instead of checking and setting things in a gitconfig to stealthily set HOMEBREW_DEVELOPER?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 7, 2016

Member

Good question. Basically because we want brew update and other things that change behaviour (e.g. printing backtraces) to be different for Homebrew developers.

This comment has been minimized.

@chdiza

chdiza Sep 8, 2016

Contributor

Yeah, but that's compatible with not using the gitconfig trick. I still don't see what the point of the gitconfig modifications are. If we want certain cmds to always exhibit dev-modeish behavior, then those cmds should just stop checking for developerhood, rather than being tricked into thinking they're dealing with a developer. The non-dev cmds can continue checking for developerhood.

@sigmavirus24

This comment has been minimized.

Copy link

sigmavirus24 commented Sep 5, 2016

So I took a look at this (I haven't downloaded it or tested it locally to be honest).

It looks like if I use something from dev-cmd then this will auto-set HOMEBREW_DEVELOPER to true for me, but I'm not intimately familiar with how brew works. I think that means the commands in this directory count towards that which explains your PR description about possibly including audit, create, edit, test, etc. (because those are in cmd).

If I'm grokking that correctly, then I'm going to request that audit, create, edit, and test be included too. I'm not sure if there are other commands in cmd that make sense because I mostly work on formulae not on brew itself.

Also, maybe you want to update the docs separately? The documentation included about bump-formula-pr was confusing at first until I realized you probably regenerated the docs. =)

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 7, 2016

It looks like if I use something from dev-cmd then this will auto-set HOMEBREW_DEVELOPER to true for me, but I'm not intimately familiar with how brew works. I think that means the commands in this directory count towards that which explains your PR description about possibly including audit, create, edit, test, etc. (because those are in cmd).

If I'm grokking that correctly, then I'm going to request that audit, create, edit, and test be included too. I'm not sure if there are other commands in cmd that make sense because I mostly work on formulae not on brew itself.

Yep 👍

Also, maybe you want to update the docs separately? The documentation included about bump-formula-pr was confusing at first until I realized you probably regenerated the docs. =)

Apologies for confusion but we do want it to be part of this PR; we tend to always update docs at the same time.

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:dev-cmd-sticky-homebrew-developer branch 2 times, most recently from 8db01cd to 0214fe0 Sep 7, 2016

@@ -0,0 +1,13 @@
module Commands
def self.path(cmd)

This comment has been minimized.

@maxnordlund

maxnordlund Sep 7, 2016

Contributor

Just some nitpicking, but I thought the other implementation, interal_command_path, was much cleaner. I never liked the not quite switch statement/list of ifs this turned out to.

@MikeMcQuaid MikeMcQuaid referenced this pull request Sep 7, 2016

Merged

dev-cmd: add `--help` to all developer commands. #890

5 of 5 tasks complete
@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 8, 2016

I'm not in favor of this PR but if you are going down this road I guess we need HOMEBREW_NO_DEVELOPER also to have a way to avoid triggering the magic.

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Sep 8, 2016

HOMEBREW_NO_DEVELOPER

I haven't tried this branch so I'm probably not entitled to comment, but HOMEBREW_NO_DEVELOPER sounds pretty ridiculous and I bet no one will ever set it.

@chdiza

This comment has been minimized.

Copy link
Contributor

chdiza commented Sep 8, 2016

HOMEBREW_NO_DEVELOPER sounds pretty ridiculous and I bet no one will ever set it.

Someone who doesn't want all their taps to automatically be full clones would set it. I would set it, for that very reason.

But I agree, if that shellvar has to be introduced, then little will have been gained by this PR.

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Sep 8, 2016

I'm a little nervous that this makes Homebrew's behavior history-dependent without clear user intent; I don't know that someone will be able to figure out why update does one thing on machine X and a different thing on a colleague's machine. Maybe the "Developer mode enabled" notification could include a link to some documentation that explains the difference?

Look at me acting like people read stderr; hope springs eternal. ;)

Would it make sense to use a different flag for the update mode? I'm not sure exactly what HOMEBREW_DEVELOPER does; it seems to do a bunch of things. I think most of them aren't disruptive, but they are different.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 8, 2016

Here's just one example of why this PR is problematic. We print a warning if your OS version is old and no longer supported by Homebrew. But if you set HOMEBREW_DEVELOPER, that message is suppressed. The fact that someone happened to have used brew edit has nothing to do with whether or not that warning should be printed.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 8, 2016

Yeah, but that's compatible with not using the gitconfig trick. I still don't see what the point of the gitconfig modifications are.

We want some behaviour to be for Homebrew developers and some behaviour to not be defaulted for them. We need to use gitconfig to set some state. At the moment instead of HOMEBREW_DEVELOPER a better description might be HOMEBREW_I_INTENTIONALLY_MAKE_GIT_MODIFICATIONS_AND_UNDERSTAND_HOW_TO_RESOLVE_PROBLEMS_THAT_RESULT_WITHOUT_NEEDING_HELP. This could have been better explained but there's basically been two contradicting forces leading to this PR:

  • Novice Homebrew users used to (relatively regularly) end up in a state where brew update couldn't update because of the state of their Git repository. Alternatively, they updated but lingering files meant Homebrew did not behave correct. Also, people would often file issues due to not runnign brew update in a while. These all produced hard-to-debug or just waste-of-time issues which seemingly entirely gone away now.
  • The new behaviour where we clean up the Git repository is surprising or annoying for Homebrew developers (e.g. anyone who makes brew/homebrew-core/tap PRs). This annoyed people a bit before but has been exacerbated by auto-update.
  • There's some commands targeted at Homebrew developers that we want more people to have access to and be documented accordingly without needing to set an environment variable.

(the above description should have been in the PR description).

@ilovezfs @tdsmith How about this: instead of setting HOMEBREW_DEVELOPER globally we have a HOMEBREW_RUN_DEVELOPER_COMMAND which changes purely the stash/reset/checkout behaviour of brew update. I think it should still also force full tap clones, for what it's worth, as you cannot create pull-requests from a shallow clone.

I'm not in favor of this PR
Here's just one example of why this PR is problematic.

@ilovezfs If you have multiple reasons/examples: please elaborate, thanks.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 8, 2016

@ilovezfs @tdsmith How about this: instead of setting HOMEBREW_DEVELOPER globally we have a HOMEBREW_RUN_DEVELOPER_COMMAND which changes purely the stash/reset/checkout behaviour of brew update

I'm not sure I understand what the name of the variable means. But yes making this a separate variable rather than overloading HOMEBREW_DEVELOPER addresses most of my concerns. Tying it to tapping would be fine I guess as long as there's an opt out envvar specifically for that. It also might be worth having it retroactively trigger an unshallowing of core.

It does not address what tdsmith already pointed out, which is the fact that brew behavior can then differ seemingly randomly between machines since this mechanism is tied only to the particular user account on a given machine. I think that will be confusing if people expect the developer behavior (since it was magically enabled for them at some point) and now it's not behaving that way and stashing changes behind their back.

It also does not address the problem that dog fooding non-developer homebrew update behavior for an extended period (without something like a HOMEBREW_NO_DEVELOPER available) would require deliberately avoiding any of the trip wires that flip it on without you explicitly asking for it.

From a review of the sources, here's what using a separate variable does indeed address in terms of unrelated behavior changes that would be avoided by that proposal:

  • prerelease xcode warning is skipped
  • deprecation exceptions all become die
  • commands with destructive options (e.g., test-bot) are unleashed on a whole new population for whom they were never intended
  • failed fetches are hard failures rather than falling through to source builds
  • go formulae will refuse to install and odie if they attempt to stage an empty resource array
  • macho parse errors all lead to a raise and a highly technical warning about failing to read Mach-O binaries
  • taps become tappable that would otherwise cause hard failures for users due to invalid syntax
  • formula unavailable errors become raises
  • every install will cause audit_installed to be called before it completes
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 8, 2016

@ilovezfs Thanks for elaborating. It sounds to me like the best middle-ground is to add a variable to opt out this behaviour, and use a non-HOMEBREW_DEVELOPER variable for this and make sure the documentation is sufficient. As to @tdsmith's point: I see the concern but I think the main thing is that we're not blowing away e.g. brew edit changes rather than maintaining consistent behaviour.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 8, 2016

It sounds to me like the best middle-ground is to add a variable to opt out this behaviour, and use a non-HOMEBREW_DEVELOPER variable for this and make sure the documentation is sufficient.

yup.

I see the concern but I think the main thing is that we're not blowing away e.g. brew edit changes rather than maintaining consistent behaviour.

If it's a separate variable, it won't be triggering as many behavior changes, so the inconsistency is greatly diminished. However, it will still be surprising if someone has things stashed simply because they sat down at a machine, double click on the rb file instead of going through brew edit, and then have a bunch of changes stashed but not popped, if they're accustomed to always having the stash-popping behavior.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 8, 2016

If it's a separate variable, it won't be triggering as many behavior changes, so the inconsistency is greatly diminished. However, it will still be surprising if someone has things stashed simply because they sat down at a machine, double click on the rb file instead of going through brew edit, and then have a bunch of changes stashed but not popped, if they're accustomed to always having the stash-popping behavior.

I agree but I'm not sure I see a better solution. The power vs. novice user imbalance we have on this project will always make this a little weird.

Thanks for help fleshing this out.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 8, 2016

Thanks for help fleshing this out.

You're welcome.

I agree but I'm not sure I see a better solution. The power vs. novice user imbalance we have on this project will always make this a little weird.

I wonder if a command front end on this (like your brew analytics on/off/state thing) would help address it since commands are so much more prominent to everyone in hands-on-use/learn-by-doing and in documentation than envvars and fine print.

@MikeMcQuaid MikeMcQuaid force-pushed the MikeMcQuaid:dev-cmd-sticky-homebrew-developer branch from 0214fe0 to 027086d Sep 8, 2016

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 8, 2016

I wonder if a command front end on this (like your brew analytics on/off/state thing) would help address it since commands are so much more prominent to everyone in hands-on-use/learn-by-doing and in documentation than envvars and fine print.

Think both that and more prominent messaging/documentation of this may be nice but I'd like to punt on it for now and see if there's any confusion with the new behaviour first (and, if so, what is it).

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Sep 8, 2016

@MikeMcQuaid

but I'd like to punt on it for now and see if there's any confusion with the new behaviour first (and, if so, what is it).

This seems pretty important, could you please give us a few days to test?

By the way, to be honest, I was never quite sure about the exact differences between with HOMEBREW_DEVELOPER and without, I just know a bunch of stuff is different, sometimes pleasantly, sometimes unpleasantly.

Edit: That is to say, definitive documentation is a must-have.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 8, 2016

This seems pretty important, could you please give us a few days to test?

I think this change should go in ASAP. We can 🚢 this as-is an iterate on it but it's still actively causing problems for contributors and I'll be happy to address further items after it's merged.

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Sep 8, 2016

Shouldn't we test first within a smaller circle who are (hopefully) intimately familiar with this PR and understand what issues it address?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 8, 2016

@zmwangx there's three groups: people who already set HOMEBREW_DEVELOPER, people who don't and are unaffected by the brew update changes in PR (the vast, vast majority of our users) and the people who don't set HOMEBREW_DEVELOPER and are surprised by brew update or brew install stashing their changes. The best way to get the latter group testing this is to release this change. It's worth nothing: this change will restore their behaviour to how it was a few weeks ago before we enabled auto-update.

@maxnordlund

This comment has been minimized.

Copy link
Contributor

maxnordlund commented Sep 8, 2016

I'm with @MikeMcQuaid here, ship it as is and then we'll get feedback in no time. While I do believe most users of homebrew are developers in one form or the other, most don't work on homebrew itself, right. So when you set HOMEBREW_DEVELOPER I do think people realize that things will act differently/may break. If you on top of that set a specific git config, I think it's safe to assume then (kinda) know what they're doing.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 8, 2016

If you on top of that set a specific git config, I think it's safe to assume then (kinda) know what they're doing.

If you've already got it set: you'll see no behaviour change. If you don't have it set and don't run the (previously undocumented) developer commands: you'll see no behaviour change. If you don't have it set, run developer commands and then run brew update: you'll see a behaviour change.

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Sep 8, 2016

That explains it. From the discussions above I kind of assumed there was more. But there's no more, so no objection to shipping. I do share Tim's concern of history-dependence though.

On Sep 8, 2016, 4:13 PM -0400, Mike McQuaid notifications@github.com, wrote:

If you on top of that set a specific git config, I think it's safe to assume then (kinda) know what they're doing.

If you've already got it set: you'll see no behaviour change. If you don't have it set and don't run the (previously undocumented) developer commands: you'll see no behaviour change. If you don't have it set, run developer commands and then run brew update: you'll see a behaviour change.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub (#881 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AD9SXD0YcTCOsWZAT3k9IK-o_lc6QHYoks5qoGx8gaJpZM4J1TwE).

@tdsmith

This comment has been minimized.

Copy link
Contributor

tdsmith commented Sep 8, 2016

Thinking about it: a doctor check that looks for the git repo flag without HOMEBREW_DEVELOPER set would be good, maybe, to explain that the user's in power mode™. This can happen post-shipping.

@ilovezfs ilovezfs referenced this pull request Sep 9, 2016

Closed

developer docs #4650

@MikeMcQuaid MikeMcQuaid merged commit 930bcb7 into Homebrew:master Sep 9, 2016

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
coverage/coveralls Coverage decreased (-0.003%) to 76.235%
Details
default Build finished.
Details

@MikeMcQuaid MikeMcQuaid deleted the MikeMcQuaid:dev-cmd-sticky-homebrew-developer branch Sep 9, 2016

@BrewTestBot BrewTestBot removed the in progress label Sep 9, 2016

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 9, 2016

Thanks for review/thoughts all. This PR is better as a result.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 9, 2016

🎉

@tbodt

This comment has been minimized.

Copy link

tbodt commented Feb 10, 2017

Is there a way to not automatically enable developer mode when running brew edit, like HOMEBREW_NO_DEVELOPER would have done?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 10, 2017

@tbodt No, sorry.

@tbodt

This comment has been minimized.

Copy link

tbodt commented Feb 10, 2017

So I'll just have to keep deleting devcmdrun from /usr/local/Cellar/.git/config...

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 10, 2017

@tbodt What specifically are you trying to do?

@tbodt

This comment has been minimized.

Copy link

tbodt commented Feb 10, 2017

Sometimes I want to see how a formula is built, so I use brew edit, but I almost never make any changes. Then I have to deal with brew updating itself more often, which is annoying because I have a really bad internet connection.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Feb 10, 2017

brew cat <formula> will serve that use case without kicking you into being a developer.

@tbodt

This comment has been minimized.

Copy link

tbodt commented Feb 10, 2017

Didn't know, thanks! Would be nicer if it opened in my editor instead, though.

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Feb 11, 2017

You can use an external edit command. For instance, put the following script

#!/usr/bin/env ruby
paths = ARGV.named.map do |name|
  path = Formulary.path(name)
  raise FormulaUnavailableError, name unless path.file?
  path
end
exec_editor(*paths)

with name brew-myedit.rb and mode 755 somewhere on your PATH. Then, invoke it as

brew myedit <formula>

This won't set devcmdrun.

Ref: External commands.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 11, 2017

@tbodt Instead set HOMEBREW_UPDATE_TO_TAG which will update less.

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.