Skip to content

Traverse upwards for magefiles #290

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ryanfaerman
Copy link
Contributor

Currently, if you attempt to run mage from somewhere other than your project root you get an error:

No .go files marked with the mage build tag in this directory.

Coming from rake, I'm used to being able to run my tasks from any level deep within my project and be confident everything will run.

This change allows the resolution of the magefiles to traverse upwards until it reaches the root /. With this update, you can run mage from any level deep of your project and it will still find the magefiles and work as expected.

Also, despite invoking at some (possibly deep) nested path, the actual binary's working directory is the directory where the magefiles are located.

Ryan Faerman added 2 commits February 10, 2020 12:57
With this change, when a user is not in the root of their mage-powered
project and executes mage, we iteratively traverse the path upwards
until we find some magefiles.

This should help with overall usability.
@ghostsquad
Copy link

@ryanfaerman I'm not saying that rake is doing it right or wrong, but if you compare to make, it does not have this behavior.

@ryanfaerman
Copy link
Contributor Author

What if I were to add an environment variable that could be used to control this behavior? Something like MAGE_RAKELIKE where it is disabled by default, but can be optionally enabled. This way if you prefer the rake behavior you can enable it, and if you prefer the default make behavior, you can have that too.

Or, this could be an option for the bootstrap.go process.

I generally prefer not to have options, but either one would be a decent compromise.

@ghostsquad
Copy link

I like that idea.

@DavidGamba
Copy link

DavidGamba commented May 25, 2020

If you consider other build systems they have the concept of a workspace.

In go, the "workspace" is where go.mod is found. If there is a go.mod in a nested dir it is considered a separate entity.

In Google's Bazel, it is defined with the WORKSPACE or WORKSPACE.bazel file.
In Facebook's Buck, it is defined with the .buckconfig file.
In please.build it is defined with .plzconfig.

There could be a mage.config file at the top of your workspace to indicate to mage how far to backtrack to.
Another alternative is to have a standard for the name of the workspace magefile and instead of introducing a new config file. You could have something like mage.go or workspace.go at the top of the workspace so mage won't recurse past that.

The difference in behaviour is that with the concept of a workspace, I would expect mage to show me targets for all directories in my workspace, not just those directly upwards in my tree.

On the other hand just going upwards without introducing the whole workspace concept might just be easier.

Copy link
Contributor

@arschles arschles left a comment

Choose a reason for hiding this comment

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

+1 on adding an environment variable. I would vote for having the behavior turned on by default, and using the env var to opt out.

Comment on lines +319 to +335
FINDFILES:
files, err := Magefiles(inv.Dir, inv.GOOS, inv.GOARCH, inv.GoCmd, inv.Stderr, inv.Debug)
if err != nil {
errlog.Println("Error determining list of magefiles:", err)
return 1
}

if len(files) == 0 {
if strings.HasSuffix(inv.Dir, "/") {
debug.Println("Reached root searching for magefiles")
} else {
inv.Dir = filepath.Dir(inv.Dir)
debug.Println("No .go files marked with the mage build tag in this directory")
debug.Println("Moving to parent to find magefiles")
goto FINDFILES
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend against using the label in favor of pulling this all out into a function + function call here. A recursion is far easier to reason about than this

Choose a reason for hiding this comment

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

It's like bigfoot. First goto I've seen in years 😆 (not saying it doesn't have it's uses, just rare to see it in the wild!)

@mirogta
Copy link
Member

mirogta commented Sep 24, 2021

Is this going to work if our magefiles are in a subdirectory, and would be for example in the ./pkg/version directory? It's the case for majority for our repositories so that we don't pollute the repo root.

@DavidGamba mage.config would make sense there, because just traversing upwards wouldn't find our mage files.

│
├─ magebuild
│  │
│  ├─ build.go
│  │
│  ├─ docker.go
│  │
│  └─ magefile.go
│
├─ pkg
│  │
│  └─ version
│
├─ main.go
│
├─ mod.go
│
├─ mod.sum
│
└─ tools.go

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.

6 participants