Bug 1790035 - Write python script to temp file, execute, then delete

[rosahbruno]

On Windows, users were unable to use the npm run glean command because the way we passed the python script as a long, template literal broke the CLI. By moving that script to its own file, executing it, then deleting it, we are able to successfully run the command.

I tested this on Mac and Windows and I am now able to get my pings file generated when running npm run glean.

This is a bug fix, it does not require documentation changes.

Pull Request checklist

• Quality: Make sure this PR builds and runs cleanly.
  • Inside the glean/ folder, run:
    • npm run test Runs all tests
    • npm run lint Runs all linters
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry to CHANGELOG.md or an explanation of why it does not need one
• Documentation: This PR includes documentation changes, an explanation of why it does not need that or a follow-up bug has been filed to do that work
  • Documentation should be added to The Glean Book when making changes to Glean's user facing API
    • When changes to the Glean Book are necessary, link to the corresponding PR on the mozilla/glean repository
  • Documentation should be added to the Glean.js developer documentation when making internal code changes

[rosahbruno]

@Dexterp37
Do you think this would be better in just its own file that we call? The reason that I am creating it locally and then removing it is so that we aren't trying to manually get to a certain path to execute the script. This goes hand-in-hand with the issue I am seeing with the build:qt command since I think that is happening because of the way we are traversing the file directories.

Either way, I think it would be good for us to chat about this and how you currently run Glean.js on your windows machine before we land these changes.

[Dexterp37]

@Dexterp37 Do you think this would be better in just its own file that we call? The reason that I am creating it locally and then removing it is so that we aren't trying to manually get to a certain path to execute the script.

I believe we should stick, for now, to generating a temporary file with the content of the script, if only for consistency with the other SDKs which have the script "embedded" in the cli/plugin used for building Glean products.

This goes hand-in-hand with the issue I am seeing with the build:qt command since I think that is happening because of the way we are traversing the file directories.

What are you seeing?

Either way, I think it would be good for us to chat about this and how you currently run Glean.js on your windows machine before we land these changes.

Happy to chat!

[rosahbruno]

@Dexterp37

I believe we should stick, for now, to generating a temporary file with the content of the script, if only for consistency with the other SDKs which have the script "embedded" in the cli/plugin used for building Glean products.

Using a temporary directory sounds like a better idea, I will make some updates.

What are you seeing?

When I try to run npm run build on my windows machine, the command fails. When you run it on windows, which shell are you using? When I try PowerShell or command prompt I get an error saying that it can't execute rm command. When I use bash, I get an issue that looks like it might be something to do with how we define the file path here https://github.com/mozilla/glean.js/blob/main/glean/package.json#L78.

[rosahbruno]

@Dexterp37
I was able to verify that this change works on both Mac and Windows. I ran into some issues with running things on Windows, but those will be addressed in a separate bug that I will be filing.

Testing inside of Glean.js

• Pull down code on windows
• Run all commands using Git Bash
• Remove the build:qt command from the npm run build script
• Then run the node sample project (you will see the generated folder is now created and you can see pings.js is there)

Testing sample project

I also copied over all the code into the sample project, patched the node_module, and then ran it again and pings.ts was created.

[Dexterp37]

Almost there! We should use log (as it's used throughout the file). We're good to go after these two last changes.

[Dexterp37]

This looks good to merge with the one final change below merged. Thanks!

[rosahbruno]

@Dexterp37
Good call, apologies for missing that. Once tests are completed, will merge. Thanks for the thoughtful reviews!

[Dexterp37]

@Dexterp37 Good call, apologies for missing that. Once tests are completed, will merge. Thanks for the thoughtful reviews!

No worries that's why we have code reviews :-)