Test for hardlink support on Windows before doing it. #3502
Conversation
|
Test failures are either known or sporadic. They are unrelated to this PR. |
pkg/runtime/links_windows.go
Outdated
| if err != nil { | ||
| logging.Debug("Test link creation failed: %v", err) | ||
| } | ||
| return err == nil |
There was a problem hiding this comment.
Please cleanup the source and target.
pkg/runtime/depot.go
Outdated
| if !d.supportsHardLinks { | ||
| return d.DeployViaCopy(id, relativeSrc, absoluteDest) | ||
| } |
There was a problem hiding this comment.
This needs to happen at a higher level. Recall we record how files were deployed in a config file. We'll need to ensure that this config has the correct value also.
There was a problem hiding this comment.
I don't see where this can happen. The next level up is in setup.go (
Lines 466 to 477 in 84273fa
Also, the storage of deployment type in a config file happens in the DeployVia*() methods, so it will have the correct value.
If there is more to do, please advise.
There was a problem hiding this comment.
ok, we're gonna have to hoist this up to setup.go then. The depots only concern is making it happen, setup.go owns the logic of interpreting envdef and invoking the appropriate deployer. We may want to revisit that given all the recent changes, but probably not for this story, unless addressing it in setup.go becomes difficult or creates other challenges.
…not, and acts accordingly.
mitchell-as
force-pushed
the
mitchell/dx-2943
branch
from
4a786e2 to
03f30b1
Compare
Naatan
left a comment
Naatan
left a comment
There was a problem hiding this comment.
Please move the hardlink detection logic and concerns to setup. Now we have an entanglement in responsibilities between setup and depot.
pkg/runtime/links_windows.go
Outdated
| logging.Debug("Determining if hard links are supported for drive associated with '%s'", path) | ||
| defer func() { | ||
| log := "Yes they are" | ||
| if !supported { | ||
| log = "No they are not" | ||
| } | ||
| logging.Debug(log) |
There was a problem hiding this comment.
| logging.Debug("Determining if hard links are supported for drive associated with '%s'", path) | |
| defer func() { | |
| log := "Yes they are" | |
| if !supported { | |
| log = "No they are not" | |
| } | |
| logging.Debug(log) | |
| defer func() { | |
| logging.Debug("Enforcing deployment via copy as hardlinks are not supported") |
pkg/runtime/setup.go
Outdated
| if err := s.depot.DeployViaLink(id, envDef.InstallDir, s.path); err != nil { | ||
| return errs.Wrap(err, "Could not deploy artifact via link") | ||
| } | ||
| } else { |
There was a problem hiding this comment.
I think you can drop this and make line 468 read
if envDef.NeedsTransforms() || !s.supportsHardlinks {
The fact that it will run ApplyFileTransforms doesn't matter, since there wouldn't be any to apply. Though for robustness it wouldn't hurt to wrap a conditional around that.
pkg/runtime/setup.go
Outdated
| } | ||
| if err := envDef.ApplyFileTransforms(s.path); err != nil { | ||
| return errs.Wrap(err, "Could not apply env transforms") | ||
| if s.supportsHardLinks { |
There was a problem hiding this comment.
| if s.supportsHardLinks { | |
| if envDef.NeedsTransforms() { |
|
Maybe this is okay now? Honestly, this PR has gone through so much churn I wouldn't be surprised if I'm still missing something. |
Uh oh!
There was an error while loading. Please reload this page.