Git Product home page Git Product logo

Comments (38)

gordonmleigh avatar gordonmleigh commented on July 18, 2024 5

@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.

from office-addin-scripts.

darrencrossley avatar darrencrossley commented on July 18, 2024 5

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.

from office-addin-scripts.

gordonmleigh avatar gordonmleigh commented on July 18, 2024 3

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.

from office-addin-scripts.

vbudovski-plexus avatar vbudovski-plexus commented on July 18, 2024 1

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

from office-addin-scripts.

denkristoffer avatar denkristoffer commented on July 18, 2024 1

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

from office-addin-scripts.

bockstaller avatar bockstaller commented on July 18, 2024 1

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?

from office-addin-scripts.

bockstaller avatar bockstaller commented on July 18, 2024 1

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.

from office-addin-scripts.

millerds avatar millerds commented on July 18, 2024 1

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.

from office-addin-scripts.

akrantz avatar akrantz commented on July 18, 2024

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.

from office-addin-scripts.

jpshankle avatar jpshankle commented on July 18, 2024

Node: 12.6.0
Yarn: 1.17.3

from office-addin-scripts.

akrantz avatar akrantz commented on July 18, 2024

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

from office-addin-scripts.

jpshankle avatar jpshankle commented on July 18, 2024

node 8 and 10 both work correctly.

from office-addin-scripts.

akrantz avatar akrantz commented on July 18, 2024

@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?

from office-addin-scripts.

jpshankle avatar jpshankle commented on July 18, 2024

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

from office-addin-scripts.

akrantz avatar akrantz commented on July 18, 2024

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.

from office-addin-scripts.

akrantz avatar akrantz commented on July 18, 2024

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

from office-addin-scripts.

akrantz avatar akrantz commented on July 18, 2024

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.

from office-addin-scripts.

denkristoffer avatar denkristoffer commented on July 18, 2024

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

from office-addin-scripts.

gordonmleigh avatar gordonmleigh commented on July 18, 2024

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

from office-addin-scripts.

akrantz avatar akrantz commented on July 18, 2024

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.

from office-addin-scripts.

lehnerpat avatar lehnerpat commented on July 18, 2024

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

from office-addin-scripts.

millerds avatar millerds commented on July 18, 2024

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).

from office-addin-scripts.

lehnerpat avatar lehnerpat commented on July 18, 2024

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).

from office-addin-scripts.

bockstaller avatar bockstaller commented on July 18, 2024

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

from office-addin-scripts.

millerds avatar millerds commented on July 18, 2024

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.

from office-addin-scripts.

bockstaller avatar bockstaller commented on July 18, 2024

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

from office-addin-scripts.

akrantz avatar akrantz commented on July 18, 2024

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.

from office-addin-scripts.

millerds avatar millerds commented on July 18, 2024

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.

from office-addin-scripts.

millerds avatar millerds commented on July 18, 2024

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).

from office-addin-scripts.

millerds avatar millerds commented on July 18, 2024

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.

from office-addin-scripts.

ChrisKader avatar ChrisKader commented on July 18, 2024

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".

from office-addin-scripts.

akrantz avatar akrantz commented on July 18, 2024

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.

from office-addin-scripts.

akrantz avatar akrantz commented on July 18, 2024

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

from office-addin-scripts.

ChrisKader avatar ChrisKader commented on July 18, 2024

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.

from office-addin-scripts.

ChrisKader avatar ChrisKader commented on July 18, 2024

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.

from office-addin-scripts.

ChrisKader avatar ChrisKader commented on July 18, 2024

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.

from office-addin-scripts.

JensMadsen avatar JensMadsen commented on July 18, 2024

temp fix (expericned tis on fresh ubuntu machine):

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

from office-addin-scripts.

millerds avatar millerds commented on July 18, 2024

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

from office-addin-scripts.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.