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

fix(npm-scripts): support windows platform using cross-var #3857

Merged
merged 6 commits into from
Jun 10, 2023

Conversation

llc1123
Copy link
Contributor

@llc1123 llc1123 commented Jun 3, 2023

Which problem is this PR solving?

Currently, the project cannot run on Windows because the npm script:

"precompile": "lerna run version --scope $(npm pkg get name) --include-dependencies"

Windows doesn't support inline shell scripts like this.
But we can use npm_package_name env var instead.
But Linux/macOS uses $ style env var references while Windows uses %% style.

Short description of the changes

  • Added cross-var as a devDependency and modified the npm script to support Windows.
  • Also fixed some minor issues. (packages missing pre-scripts).

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Run npm run compile on Windows machines.

Checklist:

  • Followed the style guidelines of this project

@llc1123 llc1123 requested a review from a team June 3, 2023 16:09
@codecov
Copy link

codecov bot commented Jun 3, 2023

Codecov Report

Merging #3857 (bd643fb) into main (228e67b) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head bd643fb differs from pull request most recent head 049ffa5. Consider uploading reports for the commit 049ffa5 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3857      +/-   ##
==========================================
- Coverage   92.98%   92.97%   -0.02%     
==========================================
  Files         300      300              
  Lines        9104     9104              
  Branches     1862     1862              
==========================================
- Hits         8465     8464       -1     
- Misses        639      640       +1     

see 1 file with indirect coverage changes

@llc1123 llc1123 force-pushed the fix/npm-script-windows branch 2 times, most recently from 8bbd6ff to c4c1621 Compare June 3, 2023 16:19
@llc1123 llc1123 force-pushed the fix/npm-script-windows branch from c4c1621 to fe7b4cb Compare June 3, 2023 16:19
@Flarna
Copy link
Member

Flarna commented Jun 5, 2023

In contrib repo we directly used the package name instead of $(npm pkg get name), see e.g. here

But more general I wonder why there are still compile and precompile scripts in each package because the actual build is done meanwhile by a typescript project in root. The compile task in root project doesn't delegate to subprojects anymore (see here).

Maybe we should just remove these npm scripts?

@llc1123
Copy link
Contributor Author

llc1123 commented Jun 5, 2023

But more general I wonder why there are still compile and precompile scripts in each package

If you want to just compile one package and its dependency, you can just go under the package directory and run npm run compile, especially when dependency compilation is required for the tests.

@llc1123
Copy link
Contributor Author

llc1123 commented Jun 5, 2023

In contrib repo we directly used the package name instead of $(npm pkg get name)

Either using env var or hard code the package name is okay.

@legendecas legendecas merged commit 4a7091f into open-telemetry:main Jun 10, 2023
@llc1123 llc1123 deleted the fix/npm-script-windows branch June 11, 2023 13:06
Nico385412 pushed a commit to Nico385412/opentelemetry-js that referenced this pull request Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants