Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

consider adding tests directory to npmignore due to windows 260 character path limitation #683

Closed
dcbarans opened this issue Feb 17, 2015 · 9 comments

Comments

@dcbarans
Copy link

As the title states. I've hit a wall where windows just falls on its face with paths longer than 260 characters. Here's an example path (not including the parent module its inside + my working directory path):

node-sass/test/fixtures/spec/spec/extend-tests/104_test_nested_extender_counts_extended_subselectors/104_test_nested_extender_counts_extended_subselectors'

The problem is compounded severely when node-sass is used as a module with other modules.

There's 2 solutions i see quickly.

  • Add tests directory to npmigore. Though not sure if that's a path you'd be willing to explore.
  • Shorten the naming convention of your test directories. Some are crazy long :)
@am11
Copy link
Contributor

am11 commented Feb 17, 2015

On Windows 8.1 x64, I keep node-sass under VS Git directory: C:\Users\Adeel\Source\Repos\node-sass. I don't have this issue. However, if we have further deep nesting this crazy limitation enforced by Windows will definitely present issues. There is only one true workaround for this issue: use longpath tool; since it is a restriction imposed by OS on shells and file-explorer not NTFS itself (you may want to take a look at the Pri.LongPath workaround described here: npm/npm#3697 (comment)).

I am afraid we cannot do much about it, because spec is a submodule in our fixture and those file names belong to spec repo: https://github.com/sass/sass-spec.

@xzyfer
Copy link
Contributor

xzyfer commented Feb 17, 2015

@am11 do the npm install scripts still need the specs out of curiosity?
On 18 Feb 2015 10:07, "Adeel Mujahid" notifications@github.com wrote:

On Windows 8.1 x64, I keep node-sass under VS Git directory:
C:\Users\Adeel\Source\Repos\node-sass. I don't have this issue. However,
if we have further deep nesting this crazy limitation enforced by Windows
will definitely present issues. There is only one true workaround for this
issue: use longpath tool; since it is a restriction imposed by OS not NTFS
itself (you may want to take a look at the Pri.LongPath workaround
described here: npm/npm#3697 (comment)
npm/npm#3697 (comment)).

I am afraid we cannot do much about it, because spec is a submodule in our
fixture and those file names belong to spec repo:
https://github.com/sass/sass-spec.


Reply to this email directly or view it on GitHub
#683 (comment).

@am11
Copy link
Contributor

am11 commented Feb 17, 2015

@xzyfer, not really. With recent modifications, we now only execute one test on npm install (via mocha code usage) to verify if the binary is compatible and whether to fallback to manual build. What do you think if we (somehow) inline that test code and add test/ to npmignore as suggested by @dcbarans?

@mgreter
Copy link
Contributor

mgreter commented Feb 18, 2015

By the way, this is a windows limit as described at: http://stackoverflow.com/a/1880453/1550314

@xzyfer
Copy link
Contributor

xzyfer commented Feb 18, 2015

Presumably would could move the tests we want into the node-sass tests suite and .npmignore sass-spec. Seems like a waste for people to download sass-spec each time they npm install, and if it's causing legitimate issues we should ditch it.

It's worth pointing out that the directory depth problem in sass-spec is going to get worse before it gets better i.e. https://github.com/sass/sass-spec/pull/262/files

@am11
Copy link
Contributor

am11 commented Feb 19, 2015

I concur. We will discard test/ directory from npm package and mocha will move down to dev-dependencies (since it will then be only required by the coverage script).

@am11 am11 added this to the 2.1 milestone Feb 19, 2015
@dcbarans
Copy link
Author

Thanks guys! I hit this error trying to install laravel-elixir which had node-sass nested 2 modules deep.. So i'm sure you will make many lives easier. Now to just get the Windows team to fix issues as quickly as you guys!

am11 added a commit to am11/node-sass that referenced this issue Feb 27, 2015
* Remove tests from publishing. sass#683
  * Uses new strategy to validate binary after
    the download.
  * Moves mocha under dev-dependencies in
    package.json.
* Truncates the forth potion from v8 version
  as discussed in sass#710.
* Moves pangyp from dev to main dependencies in
  package.json.
am11 added a commit to am11/node-sass that referenced this issue Feb 27, 2015
* Remove tests from publishing. sass#683
  * Uses new strategy to validate binary after
    the download.
  * Moves mocha under dev-dependencies in
    package.json.
* Truncates the forth potion from v8 version
  as discussed in sass#710.
* Moves pangyp from dev to main dependencies in
  package.json.

Issue URLs: sass#683 and sass#710.
PR URL: sass#717.
am11 added a commit to am11/node-sass that referenced this issue Feb 27, 2015
* Removes tests from publishing. sass#683
  * Uses new strategy to validate binary after
    the download.
  * Moves mocha under dev-dependencies in
    package.json.
* Truncates the forth portion from v8 version
  as discussed in sass#710.
* Moves pangyp from dev to main dependencies in
  package.json.

Issue URLs: sass#683 and sass#710.
PR URL: sass#717.
am11 added a commit to am11/node-sass that referenced this issue Feb 28, 2015
* Removes tests from publishing. sass#683
  * Uses new strategy to validate binary after
    the download.
  * Moves mocha under dev-dependencies in
    package.json.
* Truncates the forth portion from v8 version
  as discussed in sass#710.
* Moves pangyp from dev to main dependencies in
  package.json.

Issue URLs: sass#683 and sass#710.
PR URL: sass#717.
am11 added a commit to am11/node-sass that referenced this issue Feb 28, 2015
* Removes tests from publishing. sass#683
  * Uses new strategy to validate binary after
    the download.
  * Moves mocha under dev-dependencies in
    package.json.
* Truncates the forth portion from v8 version
  as discussed in sass#710.
* Moves pangyp from dev to main dependencies in
  package.json.

Issue URLs: sass#683 and sass#710.
PR URL: sass#717.
@am11
Copy link
Contributor

am11 commented Feb 28, 2015

This is fixed by 9bd034c via #717.

@am11
Copy link
Contributor

am11 commented Feb 28, 2015

Turned out we don't need .npmignore for this. package.json would do the trick. See fffc062#commitcomment-8369073.

Re-fixed by #724.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants