build.cmd fails if path contains a space #42397
Comments
Dotnet-GitSync-Bot
added
area-Infrastructure-coreclr
untriaged
|
I think these runtime/src/coreclr/build-runtime.cmd Lines 10 to 11 in c87e75e should probably not add quotes and instead the quotes should be applied where they are used runtime/src/coreclr/build-runtime.cmd Line 13 in c87e75e runtime/src/coreclr/build-runtime.cmd Line 724 in c87e75e Same in crossgen-corelib.cmd. Can you try that and see how it works? Building the repo when there's a space in the path breaks periodically because we dont have a test to protect it and apparently nobody that works on the repo day to day has this configuration. |
|
Adding some quotes at the right place allowed to execute few more lines without errors until the same issue happened again. I think quotes are missing at many places in the scripts and I don't feel comfortable doing the PR. It should be easy to reproduce by adding a space in the project folder though. |
|
Too bad thanks for trying. Realistically this is going to keep breaking regularly and probably not a high priority. Can you place the repo another location? |
Is having a test with a space in the path not possible?
I have the student version of Windows 10 which forces $"{firstName} {lastName}" as a username so I'll move the folder to C: or maybe it's time to get a real license 👍 |
|
We could modify one of our CI legs potentially (cc @safern) but it's probably not a priority because the workaround is pretty easy. I put my repo for example in C:\git\runtime |
After putting the repository in |
|
It seems like we need to add quotes to usages of and also to usages of |
|
I suspect this will blow up in multiple places in the scripts as we're generally not too consistent about quoting everything. I'm adding this to the infra backlog project as it definitely merits fixing, it's just likely to have quite a bit of a bug tail. |
|
I have a partial fix I am validating here: #42671.
|
This has come up and been fixed again a number of times. I suggest to either protect the next fix with CI or just document it doesn't work. |
|
I agree we should introduce lab validation for the fix, I talked with Manish about it yesterday, it's just unclear to me right now how do to that as AFAIK both the AzDO build system and Helix use an internally hardcoded scheme for the root path (that naturally doesn't contain any whitespace). In the worst case we can create a dedicated pipeline that will manually copy over the cloned repo into a new path containing blanks. Speaking about which, we might want to expand this envisioned testing to other similar cases that also tend to cause trouble like paths containing accented letters and / or Chinese / Japanese Kanji symbols. |
|
not sure about the ROI of adding a dedicated pipeline for validating this, unless this has been raised very frequently. |
|
I wonder whether we could clone the repo to a |
|
By "dedicated pipeline" I didn't mean any heavyweight machinery, I think I rather wanted to point out that it probably doesn't make sense to add this to regular PR / CI pipeline as these are already "too complex"; so I would put this either to outerloop or to some "special obscurity tests" that should suffice to run once a week or so; not sure if we already have something like that (some of the stress tests run once a week but I don't think they are the ideal place for this). Cloning the repo to a different location is certainly an option but I suspect it's relatively costly in terms of implementation as today all pipelines assume that $(Build.SourcesDirectory) is the repo root so that would need fixing globally. I guess it also depends on the envisioned scope of such testing i.o.w. whether we want just a simple canary run verifying that CoreCLR / mono / libraries build in such scenario or whether we want to e.g. build and run tests, possibly in varying configurations, in this case. |
|
ah, yeah some outerloop scenario would make sense. |
|
Do we need to fix this though? We could just update the build script (at the start, where we already validate some dependencies) to fail out with a helpful error if there's a space in the path, so folks can relocate their clone before they get very far. This assumes there aren't users who are in a locked down environment where they have to put their files under ...\Documents, though. If that turns out to be the case, then we could fix it then. I don't have a preference myself, just pointing out the work (and ongoing CI maintenance) may not be the best use of time. |
So, I've reinstalled my Windows but this time with only my first name as a username but unfortunately it contains a "é" which makes the build fail 😭 |
|
That's exactly the problem, I'm afraid people will continue hitting this. I think the CI cost is relatively low, at least w.r.t. validating that we can build CoreCLR+libraries in one configuration, with a bit of hand waving that should basically boil down to putting a modification of this line in some suitable pipeline (after the checkout to a non-default folder): |
|
Ah. I missed that it affects non-ASCII directories as well.. |
|
I tried to validate my PR with a folder named Believe this will require some CMAKE related changes, unless its just a machine config thing. |
|
This is since midl adds the full path to the idl as a comment in generated code which trips up the c++ compiler: Running this generates clrinternal.h with this (the lcid specified doesnt seem to have any effect on the output): One potential workaround could be to specify a relative path to |
|
Ok getting closer. After a tip from @davidwrighton we were able to avoid the full file path from midl. After a similar fix to python eventpipe generator I am now able to build using So I propose to merge the PR, and I am also testing some changes in CI to build x86 with a non-standard folder. |
|
Ok, looks like this msbuild issue is probably causing the issue where current chcp is not honored: dotnet/msbuild#5724. But after enabling @verdie-g if you please try whether this PR: #42671 gets things working for you we can get that merged. Thx! |
|
I've reinstalled everything and got rid of any space or non-ascii character so I won't be able to test the PR sorry |
|
Ok no worries, will validate with a few variations and merge if that goes well. |
|
keeping this open so we can track the CI changes to ensure this doesnt regress, but that will have to wait for the |
|
I have found a regression of this issue. Executing There also is a problem with generating timestamps which I have not been able to pinpoint yet. |
|
Hey @lkempf, thanks for reporting the issue. Would you like to submit a PR, assuming its a simple fix? Else will take a look later this week, we also plan to add verification for this within the CI. |
|
FYI @jkoritzinsky |
|
I've created #44540 to fix the issue I could pinpoint. |
|
@mangod9 as you are assigned to the issue, do you know what's remaining here? CI protection or an infra fix? |
|
primarily CI protection, I had a fix made a few months ago, but there is a good possibility things have since regressed. |
|
This is still a thing in as noted in #73327. I wrote a deeper explanation here #73327 (comment). |
My username is
abc defand I get an error runningbuild.cmd.After removing the line
@if not defined _echo @echo offinbuild-runtime.cmdand running the file I get the same error but with more details.It seems like there is an issue with quotes at the line
call ""C:\Users\abc def\Documents\dotnet-runtime2\src\coreclr\""\setup_vs_tools.cmd.The text was updated successfully, but these errors were encountered: