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

Allow for multiple root directories when scanning for fence files #91

Merged
merged 5 commits into from
May 6, 2021

Conversation

Adjective-Object
Copy link
Collaborator

Motivation

In owa, we currently rely on excluding the node_modules directory.

This works fine under current usage, but BuildXL by convention stores state in the Out directory, which stores unique logs across multiple runs -- this was causing good-fences to take upwards of 20 minutes on subsequent runs.

By specifying the rootDirs as packages and shared instead of relying on traversing from the common directory ., we speed up good-fences (and avoid transient read errors caused by files being deleted out from under good-fences as it traverses BuildXL's internal state).

Changes

  • changes rootDir to an array of strings
  • supports falling back to rootDir: string as an argument for backwords compatibility
  • upgrades commander to use variadic arguments to support rootDir from the cli

@smikula
Copy link
Owner

smikula commented May 5, 2021

@Adjective-Object Is there a reason to prefer multiple root directories as opposed to adding an option to exclude directories? The latter seems more intuitive to me, and more in line with something like .gitignore or .prettierignore.

I'm imagining an optional ignoreDirs option (string[]) that — in order to be backward compatible — gets autopopulated with node_modules based on the ignoreExternalFences setting. Longer term supporting something like a .fencesignore file would be great.

@Adjective-Object
Copy link
Collaborator Author

Adjective-Object commented May 6, 2021

Do you feel strongly about having an ignore file over a list of includes? As much as I like the aesthetics of .ignore files:

  • There's no ecosystem coherence, Jest has a list of roots, prettier has an .ignorefile, tsconfig has both with the more commonly used in tutorials being "include"/
  • Ignorefiles come with the expectation of git pattern matching, which means pulling in ignore, as well as supplying a way to set the ignore file from the command line.
  • Ignorefiles also mean performing a directory tree traversal to find ignorefiles when not run in a repo root. Which means our FS walk has to to worry about symlink loops, and cross-fs boundaries.
  • As you develop you are more likely to introduce more roots you want to ignore than roots you want to pay attention to (as is the case here). Moreover those new roots don't immediately cause failures, but rather cause slow performance degradation.

import RawOptions from '../types/RawOptions';
import Options from '../types/Options';
import normalizePath from './normalizePath';
import RawOptions from "../types/RawOptions";
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like I don't have prettier configured in this repo yet. For my sanity, can you use single quotes for all the string literals in this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like you beat me to it!

@smikula smikula merged commit d1c2711 into smikula:master May 6, 2021
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.

2 participants