search parent dirs for workspace

[peterbecich]

#1237

Description of the change

Search up to 4 levels up the file tree for a workspace

Stop at .git

This search only happens if a workspace not found in current working directory

Test of finding a workspace in parent dir successfully

example/spago.yaml has no workspace
example/../spago.yaml has a workspace

# current working directory is 'example'

$ ~/purescript/libraries/spago/bin/index.dev.js build
Reading Spago workspace configuration...
Looking for Spago workspace configuration higher in the filesystem, up to project root (.git)...
Checking for workspace: /home/peterbecich/haskell/libraries/purescript-bridge/example/../spago.yaml
Found workspace definition in /home/peterbecich/haskell/libraries/purescript-bridge/example/../spago.yaml

✅ Selecting package to build: example

Downloading dependencies...
Building...
           Src   Lib   All
Warnings     0     0     0
Errors       0     0     0

✅ Build succeeded.

Test of failing to find a workspace in parent directories

example/spago.yaml and example/../spago.yaml have no workspace

$ ~/purescript/libraries/spago/bin/index.dev.js build
Reading Spago workspace configuration...
Looking for Spago workspace configuration higher in the filesystem, up to project root (.git)...
Checking for workspace: /home/peterbecich/haskell/libraries/purescript-bridge/example/../spago.yaml

❌ No workspace definition found in this directory
or in any directory up to root of project.
Root determined by '.git' file.
See the relevant documentation here: https://github.com/purescript/spago#the-workspace

I have not carefully tested the search stops at .git.

Checklist:

• Added the change to the "Unreleased" section of the changelog
• Added some example of the new feature to the README
• Added a test for the contribution (if applicable)

P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊

[peterbecich]

@f-f , please review

It does work.

It should stop searching at the directory which contains .git, but I have not carefully tested this.

[peterbecich]

I think the feature to stop at .git is broken. The use of Glob is probably incorrect.

[f-f]

My understanding of this patch is that it adds functionality for finding the workspace if it's reasonably close - I have read through the code but won't review it further until we add a few important test cases that might prompt for a redesign of the patch.

In particular, there are two situations that we should cover - these are good starting points to add as tests:

1. running spago build from a folder project/b, where the package b depends on a package that is in project/a, and the workspace is in project. I suspect this will fail with the current patch, since we find the workspace file, but we don't have a complete workspace! The packages in the subdirectories from the workspace are all part of the workspace.
2. read a file in the workspace root when running spago run both from the workspace folder, and from a subdirectory. The idea is that spago run (and spago test too) need to always run from the same place, and through user testing we have found that the root of the workspace is the least surprising place

Both of these suggest that a good course of action might be to:

1. find out which package we have at hand in the spago.yaml
2. find out where the workspace is, if any
3. move the cwd there

That's what I would try, and I think it will solve most of our issues, but there might be corner cases that we'll need to address then

[f-f]

@peterbecich anything I can do to help move this forward?

[peterbecich]

I have fixed this: #1253 (comment)
It stops at .git successfully. Here is a test.

The cwd is purescript-bridge/test/RoundTripArgonautAesonGeneric/app. The non-workspace spago.yaml is there.
The workspace spago.yaml is 3 levels up in purescript-bridge.

Reading spago.yaml...
Looking for Spago workspace configuration higher in the filesystem.
Search limited to 4 levels, or project root (.git)...
Project root (.git) found at: 
/home/peterbecich/haskell/libraries/purescript-bridge/test/RoundTripArgonautAesonGeneric/app/../../../.git
Found workspace definition in: 
/home/peterbecich/haskell/libraries/purescript-bridge/test/RoundTripArgonautAesonGeneric/app/../../../spago.yaml

✓ Selecting package to build: argonaut-round-trip-test

Downloading dependencies...
Building...
           Src   Lib   All
Warnings     0     0     0
Errors       0     0     0

✓ Build succeeded.

Regarding the improvements to be done,

move the cwd there

I can't figure out how to implement this.

The PR does not fix the two cases you have identified. However it does cover the simplest case, where the workspace is several levels up from the cwd non-workspace spago.yaml.
Can we merge it as-is and cover the two cases later?
My reasoning is that it does not really introduce any regression. Instead, the feature is half-implemented.

Thanks

[f-f]

move the cwd there

I can't figure out how to implement this.

See Node.Process.chdir

The PR does not fix the two cases you have identified. However it does cover the simplest case, where the workspace is several levels up from the cwd non-workspace spago.yaml.
Can we merge it as-is and cover the two cases later?
My reasoning is that it does not really introduce any regression. Instead, the feature is half-implemented.

The two tests I mention are in fact potential regressions that I believe this patch can trigger, and we will not know if these regressions can happen until we explicitly test for them.