-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fix Sandcastle paths for windows #13026
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for the pull request, @jjspace! ✅ We can confirm we have a CLA on file for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This caused the same issue as @Beilinson already mentioned in #13019 (comment) : It says
Error: spawn C:/cesium/node_modules/typescript/bin/tsc ENOENT
in the build-sandcastle step.
Near the linked comment, you already said
I wonder if the binary isn't set to be executable on Windows
And ... this is ... "true", in some way. It is not a binary. And the concept of "being executable" does not exist on Windows in the same way as it does on Linux. The file does exist (no comment about ENOENT at this point). But it is a file that only contains
#!/usr/bin/env node
require('../lib/tsc.js')
I'd expect it to be the same on Linux. But on Linux, one can just set the "executable" flag, and then, the first line tells Linux how to execute that thing (roughly like that...)
The linked issue already suggested using
const ls = spawn(process.execPath, [binPath, "-p", configPath]);
And from my understanding, this should make sense: The process.execPath will be
C:\Program Files\nodejs\node.exe
as expected. The binPath is what is shown above. So this is explicitly using node to execute that file, which (I think) is the same what happens under Linux, but there, this is accomplished with that
#!/usr/bin/env node
line.
One important point - referring to your comment at #13019 (comment) - is that this is not using the "globally installed npx (with its arbitrary version)". It is really executing the exact tsc that is found in the node_modules directory.
Given the lack of alternatives (with the "Compiler API" being "version 0.5 beta" for 2 years now...), this sounds like a reasonabe solution, unless I'm overlooking something.
|
And I didn't mention that: The "globby" fix also seems to work - it does display the gallery. BTW:
but (as discussed elsewhere), the sequence of My "broken record" mantra about documentation and such: I think that the build infrastructure could and should(!) be simplified significantly. The 1500-line(!)-gulpfile could be shrunk to 1000 lines by removing redundancies. It should then be increased to 2000 lines by adding extensive documentation for each and every function. This 2000-line-file should then be broken down into at least 10 files with generic utility functionalities. One prominent example is the "generate third party" functionality. If this was implemented once, in a proper form, and in a standalone file, then the same file could be used in CesiumJS, in the (In fact, the |
|
But when I change to: export default async function typescriptCompile(configPath) {
const tsPath = fileURLToPath(import.meta.resolve("typescript"));
- const binPath = join(tsPath, "../../.bin/tsc");
+ const binPath = join(tsPath, "../../../.bin/tsc.cmd");
return new Promise((resolve, reject) => {
- const ls = spawn(binPath, ["-p", configPath]);
+ const ls = spawn(binPath, ["-p", configPath], { shell: true });
ls.stdout.on("data", (data) => {
console.log(`stdout: ${data}`);
});
ls.stderr.on("data", (data) => {
console.error(`stderr: ${data}`);
});
ls.on("close", (code) => {
if (code === 0) {
resolve(code);
} else {
reject(code);
}
});
});
}It works. Okay, I think I know what's going on. Using Notepad, I opened and They are actually script files with shebang lines. As we known, Windows does not support Unix shebang, so we can only call the The |

Description
tscbinary to account for "url" vs "file" path discrepancies on Windows/instead of\forglobbywhich only accepts/I am unable to easily test these changes myself as they only affected windows but I tried to validate the individual functions with fake paths as much as possible. @javagl you've seen these the most so I'd appreciate if you can validate that things are working for you now.
Issue number and link
Fixes #13019
Fixes #12951
Testing plan
npm run build-sandcastlenpm startAuthor checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change