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

[office-addin-debugging] On Mac, .bin contains CRLF instead of LF #168

Closed
akrantz opened this issue Jul 18, 2019 · 38 comments
Closed

[office-addin-debugging] On Mac, .bin contains CRLF instead of LF #168

akrantz opened this issue Jul 18, 2019 · 38 comments
Assignees
Labels
triaged Bug has been triaged

Comments

@akrantz
Copy link
Contributor

akrantz commented Jul 18, 2019

MacOS still appears to have the problem of getting
$ office-addin-debugging stop manifest.xml env: node\r: No such file or directory error Command failed with exit code 127.
The way to resolve the issue locally is to change the line ending in .bin -> office-addin-debugging from CRLF to LF.

Originally posted by @jpshankle in #160 (comment)

@akrantz
Copy link
Contributor Author

akrantz commented Jul 18, 2019

We don't create the .bin folder. It is created by "npm install" based on the "bin" in package.json.

Please let us know what versions of Node and NPM you are using:
node --version
npm version

Also, on my Mac, I have installed Node via homebrew. It would help to know how you've installed Node and whether you are also using something like NVM to be able to switch NPM versions.

@jpshankle
Copy link

jpshankle commented Jul 19, 2019

Node: 12.6.0
Yarn: 1.17.3

@akrantz
Copy link
Contributor Author

akrantz commented Jul 19, 2019

Please see if you have different results when using Node 10.

@vbudovski-plexus
Copy link

I ran into this problem too. Fixed it by changing the line endings of node_modules/office-addin-debugging/cli.js

@jpshankle
Copy link

node 8 and 10 both work correctly.

@akrantz
Copy link
Contributor Author

akrantz commented Aug 16, 2019

@jpshankle Thanks for the info. I think this is a bug with Node 12, although I couldn't find any issues related to it when searching the repository. Do you want to open an issue there? https://github.com/nodejs/node/issues

Also, do all of the node_modules/.bin files have the issue or is it specific to a particular package?

@jpshankle
Copy link

I have only run into it in the office-addin-debugging package, so I'm not sure what other packages it might be happening in

@denkristoffer
Copy link

I'm seeing the same issue with Node 10. Changing the line ending fixed it.

@gordonmleigh
Copy link

@akrantz the .bin folder is created by NPM by symlinking whatever you specify in the bin field in package.json. You have specified lib/cli.js, which is exactly what is symlinked into the .bin folder. You have CRLF endings in this file, hence the issue here.

npm/npm#12371 seems to relate, but I don't think it was ever definitively resolved (despite being marked as such). I'm not sure it can be resolved to everyone's satisfaction, because NPM shouldn't really be assuming anything about people's uploaded code.

By adding the shebang #!/usr/bin/env node to the top of this file, the authors are effectively attempting to create a *nix compatible executable file. They're not following the spec by then using CRLF endings in that file, which is why it is breaking—which is the authors' fault, not NPMs.

It is never going to be a node issue, because node doesn't come into the picture until the shell successfully parses the shebang, so node version is not relevant. It could be that previous versions of npm worked round this mistake, or it could even be that previous versions of bash on macOS could deal with CRLF, which might be why it seems to work for some.

@akrantz
Copy link
Contributor Author

akrantz commented Oct 22, 2019

OK, perhaps this is just a git issue in that the line endings are not converted to LF only on check-in, and it may be due to this one particular package which is why we don't see it consistently across packages.

@akrantz
Copy link
Contributor Author

akrantz commented Oct 22, 2019

@TCourtneyOwen I wonder if this is really a git issue or rather from the fact that we usually build and publish from Windows machines.

@akrantz
Copy link
Contributor Author

akrantz commented Oct 22, 2019

Also, this mentions that .bin is symlinked to lib/cli.js but now these should all be symlinked to cli.js, and it requires(lib/cli.js). I'm still not sure what the real issue is here because I've not had problems when using these packages on my mac with Node 10. I'm also curious if Node 12 becoming LTS will result in a different behavior.

@denkristoffer
Copy link

@akrantz Are you building these packages yourself on your Mac or are you installing from NPM?

@gordonmleigh
Copy link

I think the project just needs a .gitattributes file with say cli.js text eol=lf

@akrantz
Copy link
Contributor Author

akrantz commented Oct 22, 2019

When I had investigated this months ago, I was not able to reproduce on my Mac when installing the packages from NPM. It seemed related to the Node version at the time based on information provided. Given the latest discussion, there seems to be the possibility that because the published packages have CRLF in the cli.js or lib/cli.js or other files, this is potentially an issue. If that is really the case though it seems this would be an issue for all of the these packages which are published when run on Mac, and that does not seem to be the case. So, I feel skeptical that this is really the issue but wanted to gather the data so that I can know if there are steps we could take to resolve this issue.

@gordonmleigh
Copy link

I believe it is a yarn vs npm thing. NPM does appear to convert line endings when linking the bin files into .bin. Yarn has an outstanding issue at yarnpkg/yarn#5480. It's still on package maintainers to publish the correct line endings though, regardless of whether one package manager works around it for you.

@darrencrossley
Copy link

I also have the same issue - and can confirm it doesn't happen when using npm - only yarn.

yarn: 1.17.3
npm: 6.9 (working)
node: v10.16.3

currently I'm adding a postinstall script to our repo:

awk 'BEGIN{RS="^$";ORS="";getline;gsub("\r","");print>ARGV[1]}' ./node_modules/office-addin-debugging/cli.js
awk 'BEGIN{RS="^$";ORS="";getline;gsub("\r","");print>ARGV[1]}' ./node_modules/office-addin-debugging/lib/cli.js

But I think the true fix is as @gordonmleigh describes.

@lehnerpat
Copy link

I'm also seeing this issue on macOS with both yarn and npx. Any progress on this?

yarn: 1.22.11
npm/npx: 7.22.0
node: v16.8.0

@millerds millerds self-assigned this Nov 9, 2021
@millerds millerds added the triaged Bug has been triaged label Nov 9, 2021
@bockstaller
Copy link

I'm having the same issue and created a minimal example with Github Actions of this problem before landing here:
https://github.com/bockstaller/lineending_demo/actions/runs/1739321056

Is there any progress on this topic?

@millerds
Copy link
Contributor

I am unfamiliar with some of the intricacies of npm and yarn in relating to the .bin folder and the line endings. We have code files cli.js in the root and the cli.ts in the src folder. The lib/cli.js is generated by the build and I'm not sure how the .bin files in node_modules folder relates to any these. Which code files would need to be modified to use LF in order for the right thing to happen on mac when using yarn (note I have not tried to reproduce this myself yet).

@lehnerpat
Copy link

Without trying this out myself, it should probably be enough to convert the cli.js file itself from CRLF to LF. It seems to be one of the very few files that have CRLF EOL in the repo, e.g. the src/cli.ts file you mentioned is already LF, as is the package.json of this package.

Furthermore, the files probably should not be checked out with CRLF on Windows for building the packages for an NPM release. I.e. build them on a Linux or Mac machine or VM, in WSL, or set the Git repo to check out the files as they are in the repo (git config core.autocrlf false).

@bockstaller
Copy link

TBH this is new territory for me, but I got it running locally on my Mac

  1. Clone this [oas] and my minimal example [me] locally
  2. [me] yarn install && yarn run validate to verify that the problem persists
  3. [oas] followed this guide (on the mac only the second half, because here I am running LF per default)
  4. [oas] npm install, cd office-addin-manifest && npm install, npm run build && yarn link
  5. [me] yarn link "office-addin-manifest" && yarn run validate -> the problem no longer exists

So theoretically ™️ everything is fixed now. Assuming yarn uses the same logic everywhere and my mental model is somewhat correct.

I'll push the updated files in a minute

@millerds
Copy link
Contributor

So this is what I'm seeing and learning:

  • VS Code shows the same file's line endings in the status bar differently on a Mac vs. a PC (mac can show LF while on windows the same file shows CRLF)
  • A couple files in this repo show the same in both platforms (CRLF) and cli.js is one of those files
  • There is a git command to show the line ending information: git ls-files --eol
  • This git command shows an index ending and a working folder ending and the index ending doesn't change on the different plaforms while the working directory one does.
  • cli.js shows "mixed" line endings for both working and index

I also found the command 'git add --renormalize .' will fix the index ends to be the standard LF that all the other files list. I think this is the correct change . . . but I'm not sure how to verify it before checking it in.

bockstaller added a commit to bockstaller/Office-Addin-Scripts that referenced this issue Jan 24, 2022
Here my repo after following the guide and staging everything.
Regarding OfficeDev#168
@bockstaller
Copy link

You can validate a part of the fix like I did here bockstaller/lineending_demo@c3fca79?diff=split

I moved the build .tgz into my minimal example and install it manually, as described here. This fixes the problem in the yarn_new_package-job, while it persists in the old yarn-job where the new package isn't installed https://github.com/bockstaller/lineending_demo/actions/runs/1741951208
But this doesn't help you avoid regressions

@akrantz
Copy link
Contributor Author

akrantz commented Jan 24, 2022

Git has a way to normalize line endings for the working tree, but I don't think this is related to the issue with line endings in .bin files. The working tree will have line endings as CRLF on Windows whereas it is LF on Mac/Linux. When checked in, they are normalized to LF. However, I don't think this issue relates to the .bin files.

It's likely either because the NPM packages are built on Windows and then published to NPM, so perhaps the .bin files which are published have CRLF because of that.

It would help to have more clarity about the root cause of the problem.

@millerds
Copy link
Contributor

I'd like to investigate more and see if the source files make a difference, but I need clarity around how .bin files are generated and used and why that seems related to the problem.

@bockstaller
Copy link

The files in the bin folder are copied there according to this configuration option: https://docs.npmjs.com/cli/v8/configuring-npm/package-json#bin
This carries over the CRLF eol to the build output.

But I did find this option https://www.typescriptlang.org/tsconfig#newLine, which allows to normalize the build outputs lineendings, independent of plattform.

Testing it on my mac and windows machine showed that the build output, indeed changed from CRLF to LF with the option activated.

@millerds
Copy link
Contributor

I'm noticing that when I import office-adding-debugging to a project that on windows the file in the bin folder include 3 named office-addin-debugging, office-addin-debugging.cmd, and office-addin-debugging.ps1. The cmd file uses CRLF while the other two use LF. The code inside these files references the clit.js file in the root (which imports the one in the lib dir). But on mac it looks like a direct symlink to cli.js in the root. This is all based off using npm . . . but it seems like yarn is doing something different and I still need to try that out.

vbudovski-plexus said that changing the line endings in the root cli.js file fixed it for them and that is the file that git identified as mixed in both the index and working versions (using --renormalize updated the index case to LF).

@millerds
Copy link
Contributor

I installed and used yarn on my Mac machine and was unable to reproduce the error myself (deleted and had it create the node_modules folder and everything in it). The files in .bin looked the same as those generated by npm on both mac and winodows respectivly. Something else must be going on.

@ChrisKader
Copy link

I setup a fresh Ubuntu-20.04 WSL instance and installed node/npm with yo and generator-office. When generated a project it had various dependencies from Office-Addin-Scripts. Most of the .TS files in the various src folders (within node_modules) all where saved with CRLF. The files in node_modules/.bin are symlinks to their respective files.

As this is the first one I ran into issues with, I will use it as an example:
office-addin-lint: /home/ck/mj/node_modules/.bin/office-addin-lint -> \home\ck\mj\node_modules\office-addin-lint\lib\cli.js

\home\ck\mj\node_modules\office-addin-lint\lib\cli.js is saved as CRLF.
As a result whenever I try to execute npm run lint
I get the error /usr/bin/env: ‘node\r’: No such file or directory

If I where to guess (and a few other files have this set):

"prettier/prettier": ["error", { endOfLine: "auto" }],

I would change any endOfLine prettier setting to "LF".

@akrantz
Copy link
Contributor Author

akrantz commented Feb 9, 2022

I don't think the issue isn't what is checked out with Git (the "working tree") but what is being published to NPM. When NPM install is run, the question is where the contents of the .bin folders come from and why it's not normalized to LF.

@akrantz
Copy link
Contributor Author

akrantz commented Feb 9, 2022

Just saw the previous comment about the tsconfig newLine setting. That sounds promising.

@ChrisKader
Copy link

I don't think the issue isn't what is checked out with Git (the "working tree") but what is being published to NPM. When NPM install is run, the question is where the contents of the .bin folders come from and why it's not normalized to LF.

The lint jobs are ran before the build. Since endOfLine is set to auto, it will preserve the EOL settings for the file it is processing. If you change the endOfLine setting to LF from auto, the files that are generated in the build process (thus the .bin folder) will then have LF EOF characters.

@ChrisKader
Copy link

If the .TS file is ever saved as CRLF and the prettier rules stay as auto, then the files will never be changed to LF. Example: If you look at office-addin-lint\src\cli.ts it is saved as CRLF. As such, any file generated from it will be CRLF. Since prettier is set to auto for endOfLine, it will not change it to LF or even flag is as incorrect if npm run lint or npm run lint:fix is ran.

This will probably only be an issue if its built by someone who has core.autocrlf set to true. As on checkout: converts LF to CRLF. So when the user runs the install with that setting, they will get CRLF files. But if the package is built on a machine that has not changed core.autocrlf OR the build process runs lint:fix first (and the prettier files are chnged from auto to LF) then the resulting files will be correct.

@ChrisKader
Copy link

Building before changing endOfLine in eslint-plugin-office-addins/src/main.ts and eslint-plugin-office-addins/.eslintrc

image

Building after changing endOfLine to lf
image

Running lint:fix after changing endOfLine to lf in the root folder will fix any files you need to as well.

@millerds
Copy link
Contributor

So after coming back to this and re-thinking through the comments I think I understand things a bit better now. I was thinking about maintaining line ending in the files themselves as they were stored in github an it's influence on npm, but now I understand that it more about the build output itself. While the file contents influence that (as indicated by using prettier and some of the things I was looking at) I think the better option is the complier flag for typescript (which I didn't understand before) that makes the js file output us LF reguardless of with the original code files (i.e. working directory) have. I am make a PR the updates the tsconfig files to do this and then the next time we update the packages they should be corrected.

@JensMadsen
Copy link
Contributor

JensMadsen commented May 4, 2022

temp fix (expericned tis on fresh ubuntu machine):

  • in node_modules/.bin: cat office-addin-dev-certs | tr -d '\r' > office-addin-dev-certs

@millerds
Copy link
Contributor

This should be fixed in the current office-addin-dev-certs package (yo office output still needs to be updated).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triaged Bug has been triaged
Projects
None yet
Development

No branches or pull requests