Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XSC compiles fail if source mapping files are missing #771

Closed
cmidgley opened this issue Jan 9, 2022 · 16 comments
Closed

XSC compiles fail if source mapping files are missing #771

cmidgley opened this issue Jan 9, 2022 · 16 comments
Labels
enhancement fixed - please verify Issue has been fixed. Please verify and close.

Comments

@cmidgley
Copy link
Contributor

cmidgley commented Jan 9, 2022

Build environment: Tested on Windows and Linux
Target device: ESP32

Description
XSC fails to compile when source mapping files are referenced but not available. For example:

//# sourceMappingURL=main.js.map

If the .map file doesn't exist, or if it does exist yet the source file referenced by the map file doesn't exist, the build will fail with something similar to this:

# xsc main.xsb
#  file not found: c:\moddable\examples\helloworld\main.js.map!
NMAKE : fatal error U1077: 'c:\moddable\build\bin\win\release\xsc.EXE' : return code '0xd'
Stop.

My use case is including a third-party library in my manifest, where js files have been pre-compiled with typescript, and contain references to a .map file, but do not include the original source files. Currently my only workaround is to fork the library, and edit every source file to eliminate the map file reference.

Expected behavior
I would expect a warning, but not a failure as the code is valid (and in my case, works after editing).

Steps to Reproduce

  1. Edit the helloworld example, and add //# sourceMappingURL=main.js.map as the last line.
  2. Compile the file

Additionally, do step 1 above, but also include a valid .map file that references a source file that doesn't exist. This will also fail with an error indicating the source file couldn't be found.

@phoddie
Copy link
Collaborator

phoddie commented Jan 9, 2022

Ignoring a missing source map and generating a warning looks straightforward. I'm less sure about handling the missing source file referenced by the map. I suppose it should generate the output and xsbug will fail to load that source file later. This patch to xsc.c attempts to do both, if my understanding is correct. Close?

@cmidgley
Copy link
Contributor Author

Thanks for looking into this. The patch doesn't quite solve the problem, but I did some debugging and found that line 130 of xsc.c is calling fxRealFilepath that fails (when the JS file has //# sourceMappingURL=main.js.map as the last line). I changed this call to your new fxRealFilepathIf and the compile then works. I tried this with a very simple two-module test (where the second module has the source mapping) and before it would not compile, and now it does and it correctly links / loads the second module. I do wonder if the first use of fxRealFilepathIf is needed/correct?

Do you plan on adding this to the main code base? Or should I plan on setting up a special build that applies a patch anytime I get an updated Moddable?

@cmidgley
Copy link
Contributor Author

Also - typo in the warning message: ### warnning: sourceMap not found

@phoddie
Copy link
Collaborator

phoddie commented Jan 10, 2022

It would help me to understand the proposed change if I could see the problem. Would you mind sharing an example project? Thank you.

@cmidgley
Copy link
Contributor Author

Easy! Just use the helloworld example, and add this to the last line of main.js:

//# sourceMappingURL=main.js.map

@phoddie
Copy link
Collaborator

phoddie commented Jan 10, 2022

That's what I did to develop the patch. It works (typo and all!):

> mcconfig -d -m
# xsc main.xsb
### warnning: sourceMap not found $MODDABLE/examples/helloworld/main.js.map!
# xsl modules
### 208 instances, 503 keys, 62 colors, 0 holes
# cc mc.xs.c
# ld mc.so

But you are proposing a further change. From the output above, I'm not seeing why that's needed. Or am I misunderstanding?

@cmidgley
Copy link
Contributor Author

When I applied the patch, it failed on segment four. I hand edited it in last time, as my codebase was old - but but given this issue I wanted to get latest (took a couple hours... nothing to do with Moddable, but I had to rebuild my image). With latest, I still get the patch error on segment four:

--- xsc.c
+++ xsc.c
@@ -367,17 +394,23 @@ int main(int argc, char* argv[])
 		fclose(file);
 		file = NULL;
 		if (name) {
-			map = fxRealFilePath(parser, fxCombinePath(parser, input, name));
-			parser->path = fxNewParserSymbol(parser, map);
-			file = fopen(map, "r");
-			mxParserThrowElse(file);
-			fxParserSourceMap(parser, file, (txGetter)fgetc, flags, &name);
-			fclose(file);
-			file = NULL;
-			if (parser->errorCount == 0) {
-				map = fxRealFilePath(parser, fxCombinePath(parser, map, name));
+			char *combined = fxCombinePath(parser, input, name);
+			map = fxRealFilePathIf(parser, combined);
+			if (map) {
 				parser->path = fxNewParserSymbol(parser, map);
+				file = fopen(map, "r");
+				mxParserThrowElse(file);
+				fxParserSourceMap(parser, file, (txGetter)fgetc, flags, &name);
+				fclose(file);
+				file = NULL;
+				if (parser->errorCount == 0) {
+					char *combined = fxCombinePath(parser, map, name);
+					map = fxRealFilePath(parser, combined);
+					parser->path = fxNewParserSymbol(parser, map ? map : combined);
+				}
 			}
+			else
+				fprintf(stderr, "### warnning: sourceMap not found %s!\n", combined);
 		}
 		fxParserHoist(parser);
 		fxParserBind(parser);

My version, which I compared against latest on github and it's the same, instead has:

		if (name) {
			map = fxCombinePath(parser, input, name);
			parser->path = fxNewParserSymbol(parser, map);
			file = fopen(map, "r");
			mxParserThrowElse(file);
			fxParserSourceMap(parser, file, (txGetter)fgetc, flags, &name);
			fclose(file);
			file = NULL;
			if (parser->errorCount == 0) {
				map = fxCombinePath(parser, map, name);
				parser->path = fxNewParserSymbol(parser, map);
			}
		}

I went ahead and hand merged it, with the new version looking like this:

		if (name) {
			char *combined = fxCombinePath(parser, input, name);
			map = fxRealFilePathIf(parser, combined);
			if (map) {
 				parser->path = fxNewParserSymbol(parser, map);
				file = fopen(map, "r");
				mxParserThrowElse(file);
				fxParserSourceMap(parser, file, (txGetter)fgetc, flags, &name);
				fclose(file);
				file = NULL;
				if (parser->errorCount == 0) {
					char *combined = fxCombinePath(parser, map, name);
					map = fxRealFilePath(parser, combined);
					parser->path = fxNewParserSymbol(parser, map ? map : combined);
				}
 			}
			else
				fprintf(stderr, "### warnning: sourceMap not found %s!\n", combined);
		}

WIth this code, when I compile helloworld with just that one line added I get:

# cc xsHost.c (strings in flash)
# xsc time.xsb
# xsc timer.xsb
# xsc Resource.xsb
# xsc main.xsb
#  file not found: /root/Projects/moddable/examples/helloworld/main.js.map!
make: *** [/root/Projects/moddable/build/tmp/esp32/debug/helloworld/makefile:630: /root/Projects/moddable/build/tmp/esp32/debug/helloworld/modules/main.xsb] Error 22

@phoddie
Copy link
Collaborator

phoddie commented Jan 10, 2022

Now I see the problem. We are working on different versions of xsc.c. My mistake. We have push today that will sync things up. In that version fxCombinePath doesn't call fxRealPath. I'll go ahead and include this patch. It fixes the error on missing source maps. It can always be defined from there.

@cmidgley
Copy link
Contributor Author

Great - I was just typing up a similar response guessing that was the issue! When available I will rebuild the dev image and give it a try. Thanks!

@phoddie
Copy link
Collaborator

phoddie commented Jan 10, 2022

This is available now. Big MODDABLE SDK change to XS 11.6 so be sure to build tools and all else from scratch.

@phoddie phoddie added the fixed - please verify Issue has been fixed. Please verify and close. label Jan 10, 2022
@cmidgley
Copy link
Contributor Author

cmidgley commented Jan 11, 2022

I've built a new image based on the latest from GitHub, but I'm still having problems. On a simple test, I get:

# xsc main.xsb
#  file not found: /root/Projects/moddable/examples/helloworld/../../../../../../main.ts!
make: *** [/root/Projects/moddable/build/tmp/esp32/debug/helloworld/makefile:636: /root/Projects/moddable/build/tmp/esp32/debug/helloworld/modules/main.xsb] Error 22

See https://github.com/cmidgley/moddable-map-missing-file for the files I used. I am compiling on Linux.

I also have a larger build, and for that I am now getting this:

-- Build files have been written to: /workspace/apps/di-test/compiled/tmp/esp32/heltec_lora_32/debug/di-test/xsProj-esp32/build
# xsc @cmidgley/di/dist/esm/di-container/di-container.xsb
make: *** [/workspace/apps/di-test/compiled/tmp/esp32/heltec_lora_32/debug/di-test/makefile:869: /workspace/apps/di-test/compiled/tmp/esp32/heltec_lora_32/debug/di-test/modules/@cmidgley/di/dist/esm/di-container/di-container.xsb] Segmentation fault

I've been unable so far to reproduce the segmentation fault outside of my larger project. It's possible this is due to a different issue, as I can't correctly generate a manifest file (see #775), so perhaps since the manifest is odd it is bringing up a different (less important) bug.

I did check the source for xsc.c and verified that it is the new version.

All files are freshly compiled (actually, it's a totally fresh install as I generate everything using Docker from a pristine image).

@cmidgley
Copy link
Contributor Author

TLDR: The Segmentation Fault is directly related to the missing map file issue, but I've been unable to create a small repo to reproduce it. Recommend we resolve the map file issue first, as segmentation issue may go away.

Detailed explanation:

I have proven it has to do with the missing source file from a map, because if I remove the sourceMappingURL from the .js file, the segmentation violation goes away. However, after >4 hours of effort, I remain unable to create a small repo to reproduce the failure.

I do have it isolated to a single file in my source tree. This is the only true JS file in a package that I am using, so not surprising it's that file (the other files are TypeScript definitions). If I copy that file into a small repo, it no longer fails with segmentation fault (but does give the missing 'ts' file error). If I symbolic link to it, it segmentation faults. And as I said above, if I remove the sourceMappingURL it compiles correctly.

The obvious answer would be to not use symbolic links, but that option is not available to me. My repo is a monorepo, and I use rushjs and pnpm to manage it. These tools manage the (Node) packages using symbolic links for all modules across a monorepo. I run Moddable using this same monorepo, and use the packages system to manage libraries including Moddable libraries/source. This is a hard requirement as I cross-compile my firmware into Node for automated unit testing and and easier debugging (as well as develop non-Moddable components in this same repo).

I have tried a ton of variations of symbolic links, but have failed to reproduce the problem without using Rush/PNPM. If I simply create a symbolic link to the source / map files at the root of a small repo, it seg faults. But no matter how I try, I can't reproduce it outside of rush/pnpm (same pathing, hand created; same symbolic links; tried lots of combinations of very long paths looking for buffer overrun, etc) Likely it is something else about how they manage the duplicate packages that I haven't yet figured out.

Given that the failure seems directly related to the missing map -> source file problem, I'd like to propose we see if we can figure out that issue first? Hopefully you will get the same error with the repo I provided, but if not, next will be to figure out why I get it and you don't. I have rebuilt from latest source twice now, and verified by hand checking xsc.c that it has the changes you patched, but I still get the error of missing ts file. Hoping perhaps once this goes away, the segmentation fault will just fade into the sunset...

@phoddie
Copy link
Collaborator

phoddie commented Jan 11, 2022

@cmidgley – Thank you for your efforts to isolate this and for posting the example at: https://github.com/cmidgley/moddable-map-missing-file. The moddable-map-missing-file is different than the example given previous, which just adds a single line to main.js. The former is missing the source file referenced by the source map; the latter is missing the source map.

With moddable-map-missing-file, I see the same error that you do:

#  file not found: .../moddable-map-missing-file-main/../../../../../../main.ts!

That can be resolved by changing xsc.c starting at line 379:

				if (parser->errorCount == 0) {
					char *combined = fxCombinePath(parser, map, name);
					map = fxRealFilePathIf(parser, combined);
					if (map)
						parser->path = fxNewParserSymbol(parser, map);
					else {
						parser->lines = NULL;
						parser->path = parser->origin;
						fprintf(stderr, "### warning: file referenced by source map not found %s!\n", combined);
					}
				}

The output is this:

### warning: file referenced by source map not found ..../moddable-map-missing-file-main/../../../../../../main.ts!

This may be a variation of what you described above. That wasn't obvious to me until having the moddable-map-missing-file to try.

Does this change work for you with the moddable-map-missing-file example?

@cmidgley
Copy link
Contributor Author

The good news is that this does seem to solve the problem of the map file (at least when doing the small test project).

The bad news is I still get a segmentation fault when compiling my main code (xsc segmentation fault). If I remove the map comment, the file compiles fine. I don't know how to debug C++ on Linux properly, but I did add some debug code to xsc (added a bunch of printf's) and found that it is using the map file to locate the original source (a ts file). The seg fault occurs inside of fxParserCode which is why I started to look at which file it was processing. I have not gotten further in the debugging, but perhaps it is trying to compile that file? I saw this from some debug code in fxRealFilePathIf. As a further test, I renamed the original source file so it would not be found, and the code compiled correctly, further providing evidence that it is trying to at least process that file in some way.

Glad to try to get a core dump or something else if you want to help me with how to do it...?

I continue to try to create a small example of the failure, but no luck. I've even used the same library (with and without symbolic linking to it), with the same manifest, and it won't fail in my small test bed. If interested in looking at it, the source code I'm trying to compile is in my fork https://github.com/cmidgley/di. In there is a manifest.json file that I include from my main code that results in the problem. It's the file dist/esm/di-container/di-container.js that fails, and it resolves to src/di-container/di-container.ts via the map file.

Let me know your thoughts on the best way to proceed with this. If you prefer, we can close this issue and I can open a new one - this issue does feel different, though somehow related to the map file.

@cmidgley
Copy link
Contributor Author

cmidgley commented Jan 11, 2022

I just noticed that the compile command:

# xsc @cmidgley/di-container/di-container.xsb
make: *** [/workspace/apps/firmware/compiled/tmp/esp32/heltec_lora_32/debug/firmware/makefile:869: /workspace/apps/firmware/compiled/tmp/esp32/heltec_lora_32/debug/firmware/modules/@cmidgley/di-container/di-container.xsb] Segmentation fault
The script failed with exit code 2

is referencing a di-container.xsb file - but that file does not exist in the output directory (the directory exists but is empty). Perhaps it is removed on the compile failure, or perhaps it's actually the output file (I don't know your xsc command options). Perhaps it is a symptom to consider? Note that I am using the -o compiled option when compiling, hence the 'compiled' directory in the path (instead of the default Moddable path).

UPDATE: I think that is just the binary compiled output - likely this is nothing.

@phoddie
Copy link
Collaborator

phoddie commented Jan 11, 2022

I think we should close this issue. The problem reported is resolved.

Please open a separate issue for the other problem with instructions for reproducing it (I understand that is providing difficult).

mkellner pushed a commit that referenced this issue Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement fixed - please verify Issue has been fixed. Please verify and close.
Projects
None yet
Development

No branches or pull requests

2 participants