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

update the WorkDir (-w) flag to default to the Dir (-d) value as documented #310

Merged
merged 2 commits into from
Jul 8, 2020

Conversation

srp
Copy link
Contributor

@srp srp commented Jun 12, 2020

The code settings up the command-line switch was passing inv.Dir as the
default value, which at the time of the call is always ".". This means
that the later code that sets inv.WorkDir = inv.Dir won't run because
inv.WorkDir is not the empty string.

The command line -help documentation reads:

  -w <string>
        working directory where magefiles will run (default -d value)

But given the above, this is not the observed behavior.

Now inv.WorkDir is defaulted to the empty string, and the code works
as expected.

srp and others added 2 commits June 12, 2020 10:41
…mented

The code settings up the command-line switch was passing `inv.Dir` as the
default value, which at the time of the call is always `"."`. This means
that the later code that sets `inv.WorkDir = inv.Dir` won't run because
`inv.WorkDir` is not the empty string.

The command line -help documentation reads:

      -w <string>
            working directory where magefiles will run (default -d value)

But given the above, this is not the observed behavior.

Now `inv.WorkDir` is defaulted to the empty string, and the code works
as expected.
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.

LGTM, thanks!

@natefinch natefinch merged commit 707b7bd into magefile:master Jul 8, 2020
@natefinch
Copy link
Member

@srp - I think I actually need to revert this. Someone mentioned this is a problem for them, and upon thinking about it, I think it makes more sense the way it was before - i.e. make the default working directory the current directory where mage is running. That is just how most applications work, and it's usually what you'd want. YOu wouldn't be running it in the current directory if you didn't intend for it to run in the current directory. At least, I think so?

So I think the best thing to do is to revert this and change the docs so they say the default is ".", not the value for -d.

@srp
Copy link
Contributor Author

srp commented Jul 16, 2020

@srp - I think I actually need to revert this. Someone mentioned this is a problem for them, and upon thinking about it, I think it makes more sense the way it was before - i.e. make the default working directory the current directory where mage is running. That is just how most applications work, and it's usually what you'd want. YOu wouldn't be running it in the current directory if you didn't intend for it to run in the current directory. At least, I think so?

So I think the best thing to do is to revert this and change the docs so they say the default is ".", not the value for -d.

That line of reasoning seems reasonable. The downside of that is that it breaks existing behavior, that is anyone with existing scripts who were using "-d" previously will then be broken and will need to go through and add "-w" to get their stuff working again, and since "-w" didn't exist on older mage versions anyone with old mage clients will be broken, so on a team the whole team will need to upgrade mage at the same time. That's annoying, but I'm on a small team so not the end of the world for me.

@mirogta
Copy link
Member

mirogta commented Jul 16, 2020

We have only one project which has the magefile.go in the root of the repo - it's the repo which works as a mage targets library. We don typically run mage in this repo.

Every other project which uses this library has then a .mage directory. So in our case in 99% of cases our mage directory (-d) is always .mage and the work directory (-w) is always .. We would actually prefer the work directory not to be defaulted to the mage directory. But in any case, we abstract this into Makefiles:

MAGE = go run .mage/mage.go -d .mage
mage:
	@$(MAGE) $(args)

.DEFAULT:
	@$(MAGE) -v $@

So that we can run make mage or make mage args="-v target" or just make target and don't need to worry about the -d or -w parameters. If we need to add the additional -w argument we'd do this in all repos in the Makefile, so even if it's a breaking change don't worry we can handle it easily.

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.

3 participants