custom targets: Don't replace \\ with / #3400
Conversation
| # it's fine to always use that for arguments. | ||
| # See: https://github.com/mesonbuild/meson/issues/1564 | ||
| if '\\' in sys.argv[1]: | ||
| raise AssertionError('Found \\ in {!r}'.format(sys.argv[1])) |
There was a problem hiding this comment.
Nit: Found \\ in source argument {!r}
| raise AssertionError('Found \\ in {!r}'.format(sys.argv[1])) | ||
|
|
||
| if '\\' in sys.argv[2]: | ||
| raise AssertionError('Found \\ in {!r}'.format(sys.argv[2])) |
There was a problem hiding this comment.
Nit: Found \\ in destination argument {!r}
Might save some time debugging later.
There was a problem hiding this comment.
Makes sense, will change, thanks. Ditto for the other.
| @@ -3,4 +3,15 @@ | |||
| import sys | |||
| import shutil | |||
|
|
|||
| # If either of these use `\` as the path separator, it will cause problems with | |||
There was a problem hiding this comment.
I would consider adding a TODO comment here saying something like
It is okay to have \ as separator for non-mingw/msys Windows environment, however this test is more strict for simplicity. In that case if later for whatever reason we decide to change it we will have less doubt.
There was a problem hiding this comment.
It's not okay for those either. People do use msys tools while building with msvc, so we cannot change it anywhere.
There was a problem hiding this comment.
Eventually, if people start reporting bugs about tools that require \ path separators, we will have to think of some way to fix this. It's a hard problem.
| output : 'outfile.txt', | ||
| build_by_default : true, | ||
| command : [copy, '@INPUT@', '@OUTPUT@']) | ||
| subdir('sub1') |
There was a problem hiding this comment.
If this is a test requirement to have testfile inside subdir then add a comment explaining why. Otherwise someone will rearrange it later and this will be forgotten. Alternatively (better) consider asserting that there is at least one / in path in copyfile.py.
There was a problem hiding this comment.
I wanted the test to be in a subdir so that there's at least one path separator in @OUTPUT@. Will add a comment.
This is no longer required since we always use `/` as the path separator for file arguments on Windows now. The only effect this now has is to mangle the string args people pass to custom targets. Closes #1564
nirbheek
force-pushed
the
nirbheek/remove-custom-target-backslashes-hack
branch
from
df73a9d
to
ce9b13a
Compare
|
Test failure seems relevant. |
|
Hum, turns out we translate So this requires more research, otherwise we're bound to break something. |
|
Not planning to work on this anytime soon, and not much code in this branch either. |
This is no longer required since we always use
/as the path separator for file arguments on Windows now. The only effect this now has is to mangle the string args people pass to custom targets.Closes #1564
I verified that gstreamer builds fine even with this removed now. Also modified an existing test for this.