Make dub run --root=x default app CWD in dub CWD
#2583
Conversation
|
@WebFreak001 bug and regression fixes should always target stable really. |
WebFreak001
force-pushed
the
fix-custom-root-cwd
branch
from
bad6481
to
03ed701
Compare
|
targetting stable, added changelog entry |
WebFreak001
force-pushed
the
fix-custom-root-cwd
branch
from
03ed701
to
e79e804
Compare
|
forgot to push the actual fix in the rebase, Seems I messed up in the rebase entirely, also forgot to push my test script, I don't know what happened there. Everything should be fixed up now. |
WebFreak001
force-pushed
the
fix-custom-root-cwd
branch
from
e79e804
to
938a7fe
Compare
| // legacy compatibility, default working directory is always CWD | ||
| gensettings.overrideToolWorkingDirectory = getWorkingDirectory(); |
There was a problem hiding this comment.
I think --root should be respected (in the long run). So reverting the behavior is just to avoid breaking code. What is our path forward ?
There was a problem hiding this comment.
I think it's sane to keep it like this forever.
Using workingDirectory "." you get the behavior that workingDirectory == package directory, which you had with the breaking behavior here. So I think it's fine really
changelog/cwd-fix.dd
Outdated
| Fixed regression with `dub --root=x` from last release | ||
|
|
||
| Since last release builds via the DUB API rely on the package path or an | ||
| explicitly set working directory, instead of the apps working directory. However | ||
| in the `dub` command line tool the previous behavior was when using | ||
| `dub run --root=path`, the built app that is being run would be run from dubs | ||
| current cwd, unless `workingDirectory` is specified in the recipe. Now since the | ||
| last release this changed to the package directory (e.g. like defaulting | ||
| `"workingDirectory": "."` in dub.json), which is considered a breaking change / | ||
| regression. | ||
|
|
||
| So this release reverts back the behavior that was accidentally changed, so that | ||
| the `dub` CLI working directory is always being used as program cwd when no | ||
| working directory is set in the package recipe. |
There was a problem hiding this comment.
Reverted v1.31.0 working directory change when using `dub run --root=<value>`
dub < v1.31.0 would run applications in the working directory it was invoked in, ignoring the `--root` argument.
In v1.31.0, `dub` started to respect the `--root` argument and run programs in the requested directory.
While desirable, this change was not intended, and has now been reverted. It will be re-introduced as part of a major version upgrade.
Let me know if that works for you. I tried to rephrase the text keeping the important informations you already put in, but doing it more succinctly.
There was a problem hiding this comment.
I have adjusted it to mostly that, but omitted that it would be re-introduced. I think the workingDirectory: "." setting is a good universal solution to cover all use cases. (those who want the dub cwd as part of some CI run and those who want to control it to their package)
WebFreak001
force-pushed
the
fix-custom-root-cwd
branch
from
938a7fe
to
96f6e5a
Compare
|
Let's focus the discussions as to whether we want to keep this behavior in #2582 :) |
|
should we merge this then? @ibuclaw I don't know how the release cycle on DUB works and you have usually done it in the past, would we just tag a new minor release with this fix? |
I'll tag the current head of stable. How soon should the fix go out? |
|
it's a regression fix, so as soon as possible would be nice. I know it breaks the CI in serve-d as well, but it's an easy workaround for me. It may not be such an easy fix for other people and in general not a nice experience if you suddenly need to change your tooling code that has worked for years.
|
Fix #2582
cc @Geod24
@ibuclaw should I target this on stable, so we can make a hotfix release of DUB with this fix? Last release contained a regression / change in behavior as described in that issue, which this PR fixes and tests.
Note: I'm not fully confident in this solution until the CI has passed, but new tests pass, I tried some CWD tests and general usage seems to work.