-
Notifications
You must be signed in to change notification settings - Fork 49
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
Introduce third party notice generation #891
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Left a few comments inline
packages/vscode-extension/scripts/install-sim-server-release-build.sh
Outdated
Show resolved
Hide resolved
fi | ||
|
||
gh release download $sim_server_tag -R software-mansion-labs/simulator-server -p "THIRDPARTY.json" -D "$output_dir"/"third-party-licenses" | ||
chmod +x "$output_dir"/"third-party-licenses" |
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.
license file doesn't require "x" (execution) mode
Also, I don't think there's any need to delete the files here. When there's a problem downloading the files the command will fail. Also, before release builds we always delete whole dist
directory.
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.
I would like to keep deleting the files, because it was quite annoying to receive an error every time i tried to run the command a second time and generating the third party notice is not the only reason somebody might try to run the command. (For example this command might be useful to download the current sim-server release after working on different branch that was using other release or locally build version).
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.
Left a couple more comments inline. Looks good otherwise. We should also likely modify .vscodeignore
file which controls what files are packaged into the vsix package. I think we should skip partial notice files from third-party-licenses directory, while we currentlu include the whole dist folder
if [ -f "$output_dir/third-party-licenses/THIRDPARTY.json" ]; then | ||
echo "Removing existing file $output_dir/third-party-licenses/THIRDPARTY.json" | ||
rm -f "$output_dir/third-party-licenses/THIRDPARTY.json" | ||
else | ||
echo "THIRDPARTY.json does not exist." | ||
fi |
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.
why do we need this? can't we just rm -f
here?
|
||
gh release download $sim_server_tag -R software-mansion-labs/simulator-server -p "THIRDPARTY.json" -D "$output_dir"/"third-party-licenses" | ||
|
||
echo "Operation completed successfully." |
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.
no need for this
const outputFile = "dist/THIRDPARTYNOTICE.json"; | ||
|
||
function readJsonFiles() { | ||
return fs.promises.readdir(directoryPath, { withFileTypes: true }).then((entries) => { |
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.
I wonder if it wouldn't be better if we expected certain license files as input rather than listing directory. This would make the command fail when there is one license file missing. Without it, when the directory is empty this script will run sucesfully
"build:extension-code-debug": "esbuild ./src/extension.ts --bundle --outfile=dist/extension.js --external:vscode --format=cjs --platform=node --sourcemap && cp ./node_modules/source-map/lib/mappings.wasm dist/.", | ||
"build:extension-code": "esbuild ./src/extension.ts --bundle --outfile=dist/extension.js --external:vscode --format=cjs --platform=node --minify && cp ./node_modules/source-map/lib/mappings.wasm dist/.", | ||
"build:expo-fingerprint": "esbuild ./node_modules/@expo/fingerprint/build/sourcer/ExpoConfigLoader.js --bundle --format=cjs --outfile=dist/ExpoConfigLoader.js --platform=node --minify", | ||
"build:extension-debug": "node scripts/buildExtensionCode.mjs debug", |
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.
use ./scripts...
for consistency ?
…lder such that it lives next to LICENSE.txt in the package
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.
I went ahead and applied the changes I suggested and will merge it for the upcoming release
This PR adds a way for us to generate THIRDPARTYNOTICE.json. It adds plugins to esbuild and vite build processes and also downloads THIRDPARTY.json from simulator-server as part of
build:sim-server
command.To use the new functionality run:
npm run generate:third-party-notice
.How Has This Been Tested:
npm vscode:package