Re the CI - this is failing on our fork at spago install due to the current spago version not understanding these new config options.
Kinda want it to ignore these fixtures until we actually run the tests no?

Re the CI - this is failing on our fork at spago install due to the current spago version not understanding these new config options.

No, that's just a warning. The real error is further down below, this one:

 ✘ The following packages do not exist in your package set:
  - package-b (did you mean: package-c, package-a)

Isn't that error caused by it not being able to parse package-b/spago.yaml and therefore it doesn't think package-b exists at all?

Could not read config at path "test-fixtures/monorepo/strict-true-workspace-censored-warnings/package-b/spago.yaml"

Almost: Spago is supposed to ignore configurations that are in subfolders with respect to configurations that contain a workspace section. Somehow it doesn't see that config in test-fixtures/monorepo/strict-true-workspace-censored-warnings/spago.yaml, so it thinks that the configs for package-a and package-c belong to the current workspace.
The parsing error for package-b is just a warning:

We might have to fix this detection bug here.

Ah yea that makes sense - test-fixtures/monorepo/strict-true-workspace-censored-warnings/spago.yaml is not being successfully parsed due to:

$.workspace.buildOpts: Unknown field(s): censorProjectWarnings, censorTestWarnings

So package-a/b/c are therefore considered part of the top-level spago workspace.

Maybe here for configPathsWithWorkspaces we just need to "soft-parse" spago.yaml to know if it's a workspace or not?
Although I suspect this would harm performance as we'd have to parse it twice in that case.

Otherwise maybe we could make the decoder ignore unknown fields and log the warning in readConfig, instead of returning Left configErrors?

Maybe here for configPathsWithWorkspaces we just need to "soft-parse" spago.yaml to know if it's a workspace or not?
Although I suspect this would harm performance as we'd have to parse it twice in that case.
Otherwise maybe we could make the decoder ignore unknown fields and log the warning in readConfig, instead of returning Left configErrors?

Hmm good points. @fsoikin any preferences/suggestions on this?

Let me make sure I understand the problem correctly. In a subdirectory there may be a config file belonging to a different (presumably newer) version of Spago, which we would fail to read, and as a result consider all its projects part of the top workspace?

@fsoikin yes - more specifically, here "fail to parse" means that we reject extra fields that are present in new versions of the config file, which we parsed correctly before #1272.

I wouldn't like to roll that patch back, so @noisyscanner mentioned, we have two options here:

1. use a non-strict parser when collecting configs in the tree
2. or just parse the yaml enough to detect the workspace key

I am leaning towards option 2 since it would be cheap both performance-wise and code-wise, but looking for things I might have missed here.

I see a few more options.

1. Do what we currently do. The CI failure can be fixed by adding a dummy spago.yaml in ./test-fixtures/ with a workspace in it. That will stop Spago from parsing down into the fixture directories, which it shouldn't be doing anyway. The advantage of this option is that it's free.
2. Distinguish between "strict" and "soft" parsing errors, returning Tuple SoftErrors (Either HardErrors Config) from readConfig, and then consider only "hard" ones for the purposes of detecting a workspace. This would be quite a big lift. I'm not even sure if Data.Codec.JSON can support that. (this is your option 1)
3. Distinguish between "complete" and "preliminary" parsing, returning something like Either HardErrors { package :: Maybe (Either Errors PackageConfig), workspace :: Maybe (Either Errors WorkspaceConfig) }, and then only check up to isJust _.workspace for the purposes of detecting a workspace. This would be a smaller lift, but still would make the code quite awkward. (this is your option 2)
4. Die upon encountering an unparseable config while globbing down the tree. This is easy to implement: just replace logWarn with die.
5. Warn on unparseable config, but still don't include any projects down the tree from it, as if it contained a workspace. This is also easy to implement. At least in Run Spago from a nested directory other than workspace root #1310, not sure about the previous state of the code.

My choice would be option 4. I think it's totally reasonable to error out in this case. What would be a real-world scenario where there is an unreadable spago.yaml in a subdirectory, apart from using different versions of Spago? And I think it's totally reasonable not to support different versions within the same directory subtree.

My second choice would be option 5. It's kind of like option 4, in that it may lead to errors due to unparseable spago.yaml, but it's more forgiving, in that it might not lead to errors. It would depend on what the current command is trying to do. Like, if you're trying to build a project that is nested down the tree from an unparseable spago.yaml, Spago would refuse, claiming that such project doesn't exist, and hopefully, together with the warning about the failure to parse, you'd be able to figure out what's up.

Options 2 and 3 I don't like at all. Not only are they very labour-intensive, but they also feel like band-aids, fixing just the current symptom, and not addressing the deeper problem.

Makes a lot of sense - 2 and 3 are a lot of work for what should be a fairly uncommon case, and with 4 you are going to find out straight away if your config is broken for whatever reason.

Sounded like 1 is worth doing as well either way so I added that to this PR, like you say it shouldn't be going into those dirs anyway. That should sort the CI.

As for 4, happy to also add that here or open another PR if you'd prefer

There is already a PR that retractors config discovery quite a bit: #1310

It would be trivial to add either option 4 or option 5 there, which I'll do as soon as we come to a consensus.

@f-f as the CI is now green due to adding the dummy test-fixtures/spago.yaml (thanks @fsoikin!) shall we merge this?
Since that bug isn't really related to this PR.

@noisyscanner the bug is related to this PR: we are changing the configuration format in a way that is not forwards-compatible.

I would like Spago to be robust against this kind of configuration format evolution (which is something that we didn't do in the previous codebase, and came back to bite us), so we should handle that case. In this direction I would like to have the dummy test-fixtures/spago.yaml reverted, so that we can verify that whatever patch we add to fix this will handle this scenario.

@fsoikin:

My choice would be option 4. I think it's totally reasonable to error out in this case. What would be a real-world scenario where there is an unreadable spago.yaml in a subdirectory, apart from using different versions of Spago? And I think it's totally reasonable not to support different versions within the same directory subtree.

My preferred option would be 5, because as I said above we should be able to fail gracefully. The "automatic traversal of subfolder" is a feature that is not welcome by everyone, so we should be careful to handle all the edge cases gracefully.
Option 4 does not feel as reasonable: why can't I build my project just because Spago found a file that it didn't like? If the config is unrelated to the current project then Spago should be able to continue execution - ignoring the whole subtree where the parse error happened while warning the user about it feels like the most reasonable option.

A real world scenario (that happened to me at some point) could be multi-backend projects that need different spago versions (and compiler versions, etc) because they are dealing with different enough ecosystems.

Then it is agreed. Option 5 shall be implemented in #1310 ere next sunrise.

Done: 171b93f

Thanks both - I've reverted adding test-fixtures/spago.yaml. I guess this means this PR is now blocked by #1310 - but that's fine, it's not urgent.