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

EOL semantics by adding .gitattributes and changing tsconfig.json #1550

Merged
merged 6 commits into from
Sep 29, 2020

Conversation

MarkSeufert
Copy link
Contributor

Description:
This PR solves issue #1529, which addresses the non-normality of end-of-line (EOL) semantics between operating systems. There are two main concerns in this issue:

  1. The version script uses os.EOL. On windows, EOL could mean either CRLF or LF depending on the git setting of autocrlf. On Linux and Mac, os.EOL means LF.
    To solve this, a .gitattributes file was added in the root directory which specifies the EOL semantics for certain types of files. OS independent files should always use LF, whereas windows specific files (.bat, .cmd, etc) should use CRLF. To show that this works, I ran npm install on a windows machine and viewed one of the resulting version.ts files. This can be seen below:
    CRLF
    I then pushed it to my branch to trigger the .gitattributes functionality and then viewed the same version.ts file, seen below:
    LF
    Notice how the new file only uses LF instead of CRLF. Success!

  2. The generated js files have inconsistent line endings, which causes differences between an npm module that is published on windows instead of on linux. To solve this, I found all the tsconfig.json files and added a single line: "newLine": "LF". This means that all the generated js files inside the node_modules folders will use LF as their EOL.

@codecov
Copy link

codecov bot commented Sep 21, 2020

Codecov Report

Merging #1550 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1550      +/-   ##
==========================================
- Coverage   93.18%   93.16%   -0.03%     
==========================================
  Files         156      156              
  Lines        4854     4854              
  Branches      986      986              
==========================================
- Hits         4523     4522       -1     
- Misses        331      332       +1     
Impacted Files Coverage Δ
packages/opentelemetry-tracing/src/Span.ts 97.14% <0.00%> (-0.96%) ⬇️

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you se the option inside the tsconfig.base inside the packages folder instead ?

@MarkSeufert
Copy link
Contributor Author

Alright I moved the option into the tsconfig.base instead. I noticed that some of the tsconfig.json files import from the tsconfig.es5 file instead of the tsconfig.base... would it make sense to add the option into tsconfig.es5 as well?

@vmarchaud
Copy link
Member

@MarkSeufert Yeah i thinks so

@Flarna
Copy link
Member

Flarna commented Sep 21, 2020

I think

fs.writeFileSync(fileUrl, content.replace(/\n/g, os.EOL));
needs to be changed to use \n instead os.EOL .
os.EOL is CRLF on windows and LF on unixes.
\n is LF independent of OS used.

@dyladan
Copy link
Member

dyladan commented Sep 21, 2020

I think

fs.writeFileSync(fileUrl, content.replace(/\n/g, os.EOL));

needs to be changed to use \n instead os.EOL .
os.EOL is CRLF on windows and LF on unixes.
\n is LF independent of OS used.

This script only writes the TS files which can have an OS specific line ending I believe which will be converted by git. The output JS will still only use LF.

@Flarna
Copy link
Member

Flarna commented Sep 21, 2020

As far as I remember it's a good idea to use git add --renormalize . to convert any file currently committed with other line ending then .gitattributes requests.

@Flarna
Copy link
Member

Flarna commented Sep 21, 2020

This script only writes the TS files which can have an OS specific line ending I believe which will be converted by git. The output JS will still only use LF.

The target of this PR is to have *.ts files with LF only after a fresh checkout independent of OS. The version script should not write OS dependent line endings then.

scripts/version-update.js Outdated Show resolved Hide resolved
@MarkSeufert
Copy link
Contributor Author

Right... I forgot that was a regex haha

@dyladan
Copy link
Member

dyladan commented Sep 23, 2020

As far as I remember it's a good idea to use git add --renormalize . to convert any file currently committed with other line ending then .gitattributes requests.

@MarkSeufert can you take care of this?

@MarkSeufert
Copy link
Contributor Author

I ran git add --renormalize . but no files were changed which I'm pretty sure means all the files in the repo already have the correct file endings.

@obecny obecny added the enhancement New feature or request label Sep 29, 2020
@obecny obecny merged commit d610fb9 into open-telemetry:master Sep 29, 2020
Flarna added a commit to dynatrace-oss-contrib/opentelemetry-js-contrib that referenced this pull request Dec 18, 2020
… semantic

Apply the same changes as in open-telemetry/opentelemetry-js#1550
in this repo to improve OS independent development.
obecny pushed a commit to open-telemetry/opentelemetry-js-contrib that referenced this pull request Dec 22, 2020
… semantic (#286)

Apply the same changes as in open-telemetry/opentelemetry-js#1550
in this repo to improve OS independent development.
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
…en-telemetry#1550)

* style(opentelemetry-js): .gitattribute and tsconfig changes for EOL normalization

* fix(opentelemetry-js): moved 'newLine': 'LF' into tsconfig.base instead of individual

* fix(opentelemetry-js): changed version script to use LF instead of OS.EOL

* fix(opentelemetry-js): simplified version script
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
…en-telemetry#1550)

* style(opentelemetry-js): .gitattribute and tsconfig changes for EOL normalization

* fix(opentelemetry-js): moved 'newLine': 'LF' into tsconfig.base instead of individual

* fix(opentelemetry-js): changed version script to use LF instead of OS.EOL

* fix(opentelemetry-js): simplified version script
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants