mcconfig/mcmanifest escaping of '#' on Windows

[cmidgley]

Two small changes to mcconfig.js and mcmanifest:

1. For Windows only, change the escaping of # (used for private scope modules) to use ^ symbol instead of \.

2. Adjust to escape # only on build rules and not execution commands. Previously it adjusted the target (and not the source) on the rule and execution command lines. This change escapes both source and target, and only on rules. I believe this is correct for all make systems, but I'm only able to test on Windows.

See #1354

[phoddie]

Thanks for the Windows-only correction and for setting this up as a PR. We'll get it merged.

[cmidgley]

I found another place where the # was not escaped on TypeScript modules/files (not at all, which I think means it would not work on any platform).

[phoddie]

Thanks. That looks good too.

[phoddie]

Oops. This patch breaks mcpack on macOS.

cd $MODDABLE/examples/packages/hello
mcpack mcconfig -d -m

It fails on a file with a # in the name:

# xsc @moddable/example-hello/#test.xsb
# xsc @moddable/example-hello2.xsb
# xsc mustache.xsb
# mcrez resources
# error: no name!
make: *** [/Users/hoddie/Projects/moddable/build/tmp/mac/mc/debug/example-hello/modules/@moddable/example-hello/#test.xsb] Error 22

It works correctly without the patch.

With the patch the makefile contains:

$(MODULES_DIR)/@moddable/example-hello/\#test.xsb: /Users/hoddie/Projects/moddable/build/tmp/@moddable/example-hello/@moddable/example-hello/\#test.js
	@echo "# xsc @moddable/example-hello/#test.xsb"
	xsc /Users/hoddie/Projects/moddable/build/tmp/@moddable/example-hello/@moddable/example-hello/#test.js -d -c -e -o $(@D) -r #test

Without the patch:

$(MODULES_DIR)/@moddable/example-hello/\#test.xsb: /Users/hoddie/Projects/moddable/build/tmp/@moddable/example-hello/@moddable/example-hello/#test.js
	@echo "# xsc @moddable/example-hello/\#test.xsb"
	xsc /Users/hoddie/Projects/moddable/build/tmp/@moddable/example-hello/@moddable/example-hello/#test.js -d -c -e -o $(@D) -r \#test

The patch bypasses the escaping of targetParts. I reworked it as follows. It works on macOS. Can you try on Windows?

		for (var result of tool.jsFiles) {
			var source = result.source;
			var sourceParts = tool.splitPath(source);
			var target = result.target;
			const escapedHash = tool.windows ? "^#" : "\\#";
			target = target.replaceAll('#', escapedHash);
			var targetParts = tool.splitPath(target);
			this.line("$(MODULES_DIR)", tool.slash, target, ": ", source.replaceAll("#", escapedHash));

The echo line should be unescaped, at least on macOS. Please check if this also works on Windows. Replace this.echo(tool, "xsc ", target); with this.echo(tool, "xsc ", result.target);.

[cmidgley]

That was what I originally did, and while the makefile runs, it causes the module load to fail on Windows due to -r ^#test in the xsc command line (the resource name becomes literally ^#test since the makefile does not remove escapes in statement lines). I have pushed a change that appears to work on Windows and hopefully on Mac, but something seems wrong to me that might be worth investigating a bit more...

Starting with the line that you provided that works:

xsc /Users/hoddie/Projects/moddable/build/tmp/@moddable/example-hello/@moddable/example-hello/#test.js -d -c -e -o $(@D) -r \#test

The operational difference seems to be the -r \#test shifting to -r #test. Here is what I've either found (Windows) or assume (Mac):

• Fails on Windows (and I assume Mac): -r ^#test
• Working Windows, fails on Mac: -r #test
• Working Mac and Windows: -r \#test

What seems odd to me is that the \#test works on Mac and Windows. It's pretty clear it's not an operational escape character in these statements, else the ^ would have worked on Windows. And it's also clear that # alone isn't a comment character in those statements, as the earlier # in the filename path would have broken the command from even working.

So while using just \# appears to work on both Windows and Mac, I wonder if there might be a different bug, somewhere else in the handling of #, where the extra \ character needed to be embedded in the name for it to work (on Mac) and is masking the root cause? I'm not confident I understand this correctly, and not sure it matters since it seems to work now!

I ran this updated PR on packages\hello with mcpack and on a modified helloworld (with a single private scope module) on Windows. I've compiled my larger app, with many private scope modules, and it compiles fine but I'm unable to verify it works (due to my multiple private scope module resolution problem that I'm still working, #1358).

While looking at the code, I also noticed that the following line seems unnecessary:

var sourceParts = tool.splitPath(source);

(in mcmanifest.js around line 586)

Unless I misunderstand tool.splitPath, the output variable sourceParts is never used, and removing it has not produced any problems (with little testing). I've left it in but commented out for your consideration.

[cmidgley]

I found another use case that wasn't being handled correctly. TypeScript and Windows only.

[phoddie]

The operational difference seems to be the -r #test shifting to -r #test.

Agreed.

It's pretty clear it's not an operational escape character in these statements, else the ^ would have worked on Windows. And it's also clear that # alone isn't a comment character in those statements, as the earlier # in the filename path would have broken the command from even working.

This has me baffled too. I wonder if the make escape rules are different for makefile rules and recipes, and/or quoted strings. I looked for some explanation, but failed. This page of the GNU Make docs has a section on # which might answer all the questions for someone more versed in these details than me. For example:

Comments within a recipe are passed to the shell, just as with any other recipe text. The shell decides how to interpret it: whether or not this is a comment is up to the shell.

Yes, the call to tool.splitPath(source); seems unnecessary, since the result isn't used.

From a purely style perspective, maybe there should be a tool.escapedHash to consolidate that, similar to tool.slash.

I will test your changes out on macOS in the next day or so. I'd like to get this landed to be able to move on to the other topic.

[phoddie]

This is working well for me. The generated makefile output also matches what I would expect.

So, two possible changes to finish this up:

• tool.escapedHash
• fully remove splitPath(source)

[cmidgley]

I've found at least two more cases where it's not being handled correctly. Both are caused by attempting to preload a file with # in it.

The first error was fairly easy to find. It's in mcconfig.js (about line 115) where the PRELOADS need to be escaped. This one also will need the escapedHash ... so moving it to tool is a good plan.

However, even with that fixed, it fails to link. I'm getting closer to figuring that out, but will need more time.

P.S. I'm heading out on vacation for about 10 days starting early tomorrow. I'll get some time in most days, but substantially less than normal.

[cmidgley]

Just updated the PR as suggested, along with the new change for preload.

The link time error I reported had nothing to do with escaping hashes. I'm debugging multiple problems at once (module resolution work) and the error was preload not resolving my module and not due to a #.

Edit: Can you test that preload is working on Mac? The change adds the escape on all platforms.

[phoddie]

Just did a quick test of preload on macOS. It works with your change and fails without it. Good catch.

Enjoy your vacation. We'll continue making progress as time allows.

[phoddie]

Merged.