search

nim-lang / atlas

The Atlas Package cloner. It manages an isolated workspace that contains projects and dependencies.
MIT License
98 stars 14 forks source link

`detectWorkspace` now returns the first ws-subdir #76

Closed ZoomRmc closed 1 year ago

ZoomRmc commented 1 year ago

While falling back to traversing subdirectories, detectWorkspace now returns immediately when a workspace is found.

Why does atlas look there anyway? It was a bit confusing when I tried to atlas use outside my workspace and it worked. Outputting (.) in info for a workspace path doesn't help.

ZoomRmc commented 1 year ago

Why does atlas look there anyway? It was a bit confusing when I tried to atlas use outside my workspace and it worked.

@elcritch, could you clarify the logic for me?

elcritch commented 1 year ago

It enables doing "vendoring", e.g. using a sub-directory as a workspace: https://github.com/nim-lang/atlas#vendoring-with-atlas

Though yes it would behave a bit surprising to users if run in the parent folder of a workspace. 🤔 A worse effect would be if a user has multiple workspaces in that parent folder. It'll just choose the "first" or "last" one and modify it.

Few options I see that we could do:

  1. Change this logic so that if multiple sub-dirs have workspace's then it'll error out. It's not a valid vendoring setup.
  2. More generically, require an atlas.link or similar to point to a given workspace. Similar but simpler than Nimble's old links in that it'd not affect packages, just where to find the workspace.
  3. Change vendoring to use a workspace in the project dir and change docs to explain using a "deps" dir. @ZoomRmc your packages PR helps that, and I think replay and all should work with it too now.

I sorta like Option 2 as a general use case (e.g. able to link random projects to different workspaces). Option 3 seems ok now. @Araq thoughts?

elcritch commented 1 year ago

Outputting (.) in info for a workspace path doesn't help.

Yah that's annoying. I started working on fixing that and other log naming issues but it started leading to too many random changes. The basic fix should be adding a line in the logic of "if msg.path == c.workspace: don't use relative path".

ZoomRmc commented 1 year ago

Though yes it would behave a bit surprising to users if run in the parent folder of a workspace. 🤔 A worse effect would be if a user has multiple workspaces in that parent folder. It'll just choose the "first" or "last" one and modify it.

Yeah, this is clearly a buggy behaviour and it will happen regularly - people most definitely will keep their projects/workspaces together.

Few options I see that we could do:

  1. Change this logic so that if multiple sub-dirs have workspace's then it'll error out. It's not a valid vendoring setup.

Nope, please :) This breaks basic use-case. Although, the valid directory structure this will happen in it's probably a bit unconventional: it needs to be a big workspace which contains multiple sub-workspaces. Although, if atlas ever treats "no explicit ws = global ws` this becomes very conventional.

  1. More generically, require an atlas.link or similar to point to a given workspace. Similar but simpler than Nimble's old links in that it'd not affect packages, just where to find the workspace.

  2. Change vendoring to use a workspace in the project dir and change docs to explain using a "deps" dir. @ZoomRmc your packages PR helps that, and I think replay and all should work with it too now.

I sorta like Option 2 as a general use case (e.g. able to link random projects to different workspaces).

I think it would be proper to follow the principle of least astonishment: vendoring is a special case and atlas should require some explicit action on user's part to enable it, while a basic setup should not expect an inverted structure. Atlas already has it's own "config" in nim.cfg, maybe this is the place to mark the project as the one using vendoring.

I like the basic idea that atlas works non-intrusively and doesn't really make the project depend on it besides managing the dependencies, I think any solution should adhere to that.