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

Pass MAGEFILE_GOCMD to compiled binary #210

Merged
merged 2 commits into from
Mar 14, 2019

Conversation

ihgann
Copy link
Contributor

@ihgann ihgann commented Dec 13, 2018

Pass the MAGEFILE_GOCMD environment variable into the compiled binary
when running with -gocmd set.

Testing Done:

  • Validated that running mage -debug -gocmd <go-runtime> <target>
    properly sets the environment variable MAGEFILE_GOCMD=<go-runtime>
    into the temporary binary.
  • Validated that compiled magefiles will not persist the initial
    -gocmd when executing - though this is still toggleable by providing
    the environment variable manually.

fixes #208

Pass the `MAGEFILE_GOCMD` environment variable into the compiled binary
when running with `-gocmd` set.

Testing Done:

- Validated that running `mage -debug -gocmd <go-runtime> <target>`
  properly sets the environment variable `MAGEFILE_GOCMD=<go-runtime>`
  into the temporary binary.
- Validated that compiled magefiles will not persist the initial
  `-gocmd` when executing - though this is still toggleable by providing
  the environment variable manually.

fixes magefile#208
@ihgann
Copy link
Contributor Author

ihgann commented Dec 13, 2018

@natefinch While this works, I have a few hesitations about it and don't know about the design you have in mind either.

If I have the following target in my magefile:

func CheckGoCmd() {
    fmt.Println(fmt.Sprintf("Go runtime is: %s", mg.GoCmd()))
}

Then the following output occurs:

> mage checkGoCmd
Go runtime is: go
> mage -gocmd /usr/local/bin/go checkGoCmd
Go runtime is: /usr/local/bin/go

These seem good.

> mage -gocmd /usr/local/bin/go -compile ./magew
> ./magew checkGoCmd
Go runtime is: go

This is still overwritable with the environment variable:

> MAGEFILE_GOCMD=/usr/local/bin/go ./magew checkGoCmd
Go runtime is: /usr/local/bin/go

tldr; the gocmd variable is not persisted into the compiled binary. I think this is proper in my opinion, as a binary creator might not want to establish the requirement of the go runtime being in a specific path. However, wanted to note this before you considered accepting, as I don't know the overall vision you might have for such a situation like this.

@natefinch
Copy link
Member

Yeah..... the thing of it is, the MAGEFILE_GOCMD environment variable is really only meant for internal use. It wasn't really built to be used by magefiles themselves. I think it was a mistake to put it in the mg package.... but it's there now and I can't take it out, so I might as well make it consistent I guess. :)

I think this is all fine. It definitely should not be hard coded into the compiled binary.

@ihgann
Copy link
Contributor Author

ihgann commented Dec 14, 2018

Cool - that makes sense. I'll probably switch to a different method locally (ideally if #24 ever gets implemented I can switch to args), but at least this should fix existing expectations.

@natefinch
Copy link
Member

just FYI, I've been sick for the past 8 days, but I fully intend on reviewing this as soon as my brain is back to normal.

Copy link
Member

@natefinch natefinch left a comment

Choose a reason for hiding this comment

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

👍

@natefinch natefinch merged commit 00c8eff into magefile:master Mar 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-gocmd should pass MAGEFILE_GOCMD to the Magefile
2 participants