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

Decouple AutoDeps from linter #334

Merged
merged 4 commits into from
Nov 13, 2015

Conversation

jacereda
Copy link
Contributor

No description provided.

@jacereda
Copy link
Contributor Author

If at some point you want to get rid of tracker.exe it would be easier to make AutoDeps faster (no need to parse/filter writes).

@ndmitchell
Copy link
Owner

Can you think of any reason not to get rid of Tracker? Is there anything it does that autodeps doesn't? Open-source cross-platform legally-distributable code seems like a big win.

@jacereda
Copy link
Contributor Author

Here is a list:

  • If MS decides at some point to make injecting code harder fsatrace could be in trouble until it gets fixed.
  • Double-checking results with both tools could help spotting bugs in fsatrace.
  • Some external tool failing when traced with fsatrace and working under tracker, although I've only experienced the reverse case (tracker failing with mingw/cygwin tools, probably 32 bit versions).

I'm not particularly worried about those, but you might think otherwise.

@jacereda
Copy link
Contributor Author

Maybe we could just use fsatrace for AutoDeps and keep both options for linting. That way, running something with AutoDeps and linting with tracker could spot some discrepancies.

@jacereda
Copy link
Contributor Author

That would imply running the command twice when both options are in place, maybe not a good idea.

@jacereda
Copy link
Contributor Author

Hmmm, now I'm thinking the best option is to just leave the listing part as it is and provide a faster AutoDeps, I'll attempt that this afternoon.

@ndmitchell
Copy link
Owner

If MS decides at some point to make injecting code harder fsatrace could be in trouble until it gets fixed.

Tracker works the same way, as do lots of things like process explorer, so I imagine this would be a bit of a nasty break.

Double-checking results with both tools could help spotting bugs in fsatrace.

Sounds like a good reason to perhaps use Tracker in the fsatrace test suite, not integrated into Shake.

Some external tool failing when traced with fsatrace and working under tracker, although I've only experienced the reverse case (tracker failing with mingw/cygwin tools, probably 32 bit versions).

I don't expect either approach to be perfect, given what it's doing.

Please remove LintTracker if you want to.

PS. I'm somewhat under the weather with cold/flu style stuff, so I'm probably not going to be in a fit state to review code to the necessary level of attention to detail until after a nap or tomorrow. I'm giving it a quick skim though, and will yell if anything looks wrong.

@jacereda
Copy link
Contributor Author

OK, let's see how it would look like without tracker...

@jacereda
Copy link
Contributor Author

With fe1c4f4 I still get excessive allocation/time in AutoDeps:

withCreateProcess.\        General.Process             29.6    3.4
commandExplicit.ham        Development.Shake.Command   17.5   31.1
forkWait.\                 General.Process             12.3    7.2
getFileInfo.\.\            Development.Shake.FileInfo   5.4    0.0
commandExplicit.autodeps.\ Development.Shake.Command    4.0   10.0
parseFSAT                  Development.Shake.Command    2.9    8.0
commandExplicit.withTemp   Development.Shake.Command    2.8    0.7
actionBoom                 Development.Shake.Core       2.1    0.1
...

Any idea?

ndmitchell added a commit that referenced this pull request Nov 13, 2015
@ndmitchell ndmitchell merged commit 286f68b into ndmitchell:master Nov 13, 2015
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.

None yet

2 participants