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: Make sure glob and minimatch are case insenitive on win32 #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

j03m
Copy link

@j03m j03m commented Jan 15, 2020

Supplies nocase option to minimatch and glob when on win32.
Additionally changes test-excludes extension check to be case
insenitive. Adds tests to support this behavior.

Supplies `nocase` option to minimatch and glob when on win32.
Additionally changes `test-excludes` extension check to be case
insenitive. Adds tests to support this behavior.
@j03m
Copy link
Author

j03m commented Jan 15, 2020

In reference to: #43

CC: @bcoe

@j03m
Copy link
Author

j03m commented Jan 31, 2020

@bcoe brought to my attention that win32 can have case sensitivity toggled on. See bcoe/c8#192 (comment). I need to consider this.

@bcoe
Copy link
Member

bcoe commented Feb 3, 2020

@j03m I did a tiny bit of research, I like the idea that this would just be a flag we set in c8 and nyc -- I couldn't find an easy way to detect this.

@coreyfarrell
Copy link
Member

Using this option would make the nyc configuration system specific. Couldn't we detect this with the following:

'use strict';

let memoized;

function isCaseInsensitive() {
  if (typeof memoized !== 'undefined') {
    return memoized;
  }

  let checkFile = __filename.toUpperCase();
  /* istanbul ignore next: paranoid check */
  if (checkFile === __filename) {
    checkFile = __filename.toLowerCase();
  }

  try {
    const stats = fs.statSync(checkFile)
    memoized = stats.isFile();
  } catch (error) {
    memoized = false;
  }

  return memoized;
}

module.exports = isCaseInsensitive;

In theory if this were in a lower-case-filename.js then __filename.toUpperCase(); could be used unconditionally but the toLowerCase fallback seems like a reasonable paranoid check.

@j03m
Copy link
Author

j03m commented Feb 5, 2020

I think what @coreyfarrell suggests is clever and could work. I will test and I'm happy to adjust the PR in this way (and the c8 PR).

However, let me take us on a tangent for a moment. For clarity, I discovered this issue when using c8 report --all against files generated by non-node/non-web environment. My raw coverage JSON's are generated by tool with a custom embed of v8. This is making me wonder if we should correct this at all, or wait to see if other members of the community encounter this issue. I worry about complicating both tools (c8 and nyc) for a situation that may not exist outside of my environment, which I can arguably correct there. While I could argue that the paths we use are "legal" I'm not sure that you would see this issue outside of a custom embed.

What do you think? Would we see this in the wild? Is this worth doing?

CC: @bcoe

@coreyfarrell
Copy link
Member

I have seen istanbuljs/nyc#1265, not sure if this is related. Essentially under Windows someone was performing require('./SomePath/File.js') but in other places performing require('./somepath/file.js'). Node.js had no problem loading because Windows is ridiculous, and nyc reported these as two separate files. IMO this is actually correct for nyc or c8 as it resulted in two instances of the module being loaded. If a user performs an require with missmatched case that is a bug in their code (it won't work on any POSIX platforms).

@bcoe
Copy link
Member

bcoe commented Feb 6, 2020

@j03m @coreyfarrell I'm open to the clever approach, with some mild concerns that we might find the problem is harder than we expect.

I'd suggest that we immediately break it out as its own module in the Istanbul organization. @coreyfarrell cool if I create one and give @j03m write access, perhaps detect-fs-case?

@coreyfarrell
Copy link
Member

I'm fine with however you want to move forward. One thing I'd say is that I want the fs access checks to be restricted to win32 only.


module.exports = {
isOutsideDir(dir, filename) {
return !minimatch(path.resolve(dir, filename), path.join(dir, '**'), minimatchOptions)
Copy link
Member

Choose a reason for hiding this comment

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

Fwiw, this still seems to the wrong result for me on Windows when dir is relative ... I think if we want to resolve one path, we should also do so for the other?

Suggested change
return !minimatch(path.resolve(dir, filename), path.join(dir, '**'), minimatchOptions)
return !minimatch(path.resolve(dir, filename), path.resolve(dir, '**'), minimatchOptions)

@connor4312
Copy link

connor4312 commented Feb 7, 2024

This still seems to be an issue in 2024 🙂 bcoe/c8#183 (comment) Would love to see some movement. Happy to try to update this PR / submit a new approach if there's changes that are needed.

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

Successfully merging this pull request may close these issues.

5 participants