Use volume-specific depot for Windows runtimes not on the main volume. #3432
Conversation
mitchell-as
commented
Aug 1, 2024
•
mitchell-as
commented
 DX-2943 We support setting up a runtime on the non-windows drive
|
mitchell-as
force-pushed
the
mitchell/dx-2943
branch
from
e0521a2
to
d1ce45d
Compare
pkg/runtime/runtime.go
Outdated
| // Windows does not support hard-linking across drives, so determine if the runtime path is on a | ||
| // separate drive than the default depot path. If so, use a drive-specific depot path when | ||
| // initializing the depot. | ||
| runtimeVolume := filepath.VolumeName(path) | ||
| storageVolume := filepath.VolumeName(storage.CachePath()) | ||
| if runtimeVolume == storageVolume { | ||
| runtimeVolume = "" | ||
| } |
There was a problem hiding this comment.
I think this logic can live within depot, no need for runtime to deal with these concerns.
There was a problem hiding this comment.
Also does this need to be windows specific logic? What does filepath.VolumeName() do on non-windows systems? Probably safer to avoid using this logic outside of windows, even if the APIs are satisfied.
There was a problem hiding this comment.
The documentation says that filepath.VolumeName() returns the empty string for non-Windows OSes. However, I can wall it off behind a runtime.GOOS check.
| @@ -46,7 +46,8 @@ func New(path string) (*Runtime, error) { | |||
| return nil, errs.Wrap(err, "Could not create runtime directory") | |||
| } | |||
|
|
|||
| depot, err := newDepot() | |||
| runtimePath := path | |||
| depot, err := newDepot(runtimePath) | |||
There was a problem hiding this comment.
Originally I wanted to pass path to newDepot(), but it didn't read right, so that's why I did the logic the way you originally saw it. I agree that the runtime shouldn't be concerned with what the depot can or cannot do, so I came up with something more readable here.
| // Windows does not support hard-linking across drives, so determine if the runtime path is on a | ||
| // separate drive than the default depot path. If so, use a drive-specific depot path. | ||
| if runtime.GOOS == "windows" { | ||
| runtimeVolume := filepath.VolumeName(runtimePath) | ||
| storageVolume := filepath.VolumeName(storage.CachePath()) | ||
| if runtimeVolume != storageVolume { | ||
| depotPath = filepath.Join(runtimeVolume+"\\", "activestate", depotName) | ||
| } | ||
| } |
There was a problem hiding this comment.
I can see where you were coming from, it feels awkward dealing with the runtimePath here, since depot shouldn't have any concerns about that. But this might be an ok compromise for now, the only other solution I see is introduce some new construct that is responsible for this information, which feels like overkill for the current use-case.
That said, may be good to highlight this compromise as a comment so we hopefully avoid us further building on this by accident. ie. if we need to extend this logic then we should probably think more about the separate construct.
|
@Naatan I'm not sure what you're asking for. Are you asking for a If the former, what calls the validator and where? This is my first experience with the new runtime. I'm a n00b. |
|
@Naatan ping |
|
@mitchell-as sorry; I meant the latter; add some code to the two deploy functions to protect against miss-use. I suspect the low level error we would encounter would not be intuitive to interpret otherwise. |
|
Thanks. Turns out that in order to avoid code duplication, I ended up doing both, heh. |
