search

aras-p / ClangBuildAnalyzer

Clang build analysis tool using -ftime-trace
The Unlicense
970 stars 61 forks source link

Make --start step optional #19

Closed i-ky closed 3 years ago

i-ky commented 4 years ago

More often than not, I need to analyze the whole artifacts directory. Making session start step optional means that I can achieve this goal easier and no extra files need to be created in the process.

ben-craig commented 4 years ago

I approve of the idea (not that it matters, I'm not a maintainer). What I've done locally is just to put down a timestamp file with the contents "0", and continuously analyzed with that.

May I suggest a slightly different approach though? Perhaps add a new command line option --gather_all, or something along those lines? My concern is that making the timestamp file read optional in the --stop path will mask users' bugs when they actually want the timestamp behavior. If you keep the flags separate, then a missing file during --stop can still be an error.

i-ky commented 4 years ago

Perhaps add a new command line option --gather_all, or something along those lines?

@ben-craig, will this option complementary to --stop or an alternative?

$ ClangBuildAnalyzer --stop --gather_all ...

vs.

$ ClangBuildAnalyzer --gather_all ...
ben-craig commented 4 years ago

I'm imagining it being an alternative. If you want incremental timings, then your build system can use --start and --stop. If you want to pick up everything, you use --gather_all.

aras-p commented 4 years ago

Yeah I'd also have the new functionality as an alternative to start/stop.

i-ky commented 4 years ago

@aras-p, @ben-craig, OK, got it! I'll rework the solution.

In the meantime, @aras-p, could you please investigate why Windows build is failing? I suspect it's not due to my changes:

Run call "C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Auxiliary\Build\vcvars64.bat" && msbuild.exe projects/vs2017/ClangBuildAnalyzer.sln /p:Configuration=Debug /p:CL_MPCount=2
At D:\a\_temp\0b5318fa-7db8-46e7-95e6-b268083d9373.ps1:2 char:103
+ ... al Studio\2017\Enterprise\VC\Auxiliary\Build\vcvars64.bat" && msbuild ...
+                                                                ~~
The token '&&' is not a valid statement separator in this version.
At D:\a\_temp\0b5318fa-7db8-46e7-95e6-b268083d9373.ps1:3 char:103
+ ... al Studio\2017\Enterprise\VC\Auxiliary\Build\vcvars64.bat" && msbuild ...
+                                                                ~~
The token '&&' is not a valid statement separator in this version.
+ CategoryInfo          : ParserError: (:) [], ParseException
+ FullyQualifiedErrorId : InvalidEndOfLine

##[error]Process completed with exit code 1.
aras-p commented 4 years ago

In the meantime, @aras-p, could you please investigate why Windows build is failing

@i-ky yeah looks like Github Actions changed the default shell from cmd to powershell, without telling anyone 🤷‍♀Now fixed on master branch here: https://github.com/aras-p/ClangBuildAnalyzer/commit/942271b99f12

ArekSredzki commented 4 years ago

This would be super useful, any updates?

mathbunnyru commented 4 years ago

@i-ky could you please finish this beautiful pull request? The feature is really great.

i-ky commented 4 years ago

@ArekSredzki, @mathbunnyru, thank you for a reminder! I have not been using the tool for a while and this PR fell out of my view. Hopefully by the end of week I will be able to spend some time on it.

mathbunnyru commented 4 years ago

@i-ky :)

pseyfert commented 4 years ago

I see github already noticed my commits. Anyway, I took the liberty to modify @i-ky 's work as discussed here.

In https://github.com/pseyfert/ClangBuildAnalyzer/pull/new/19-history I added a commit that restores the --stop behaviour in the current master. Ignoring the --start step is an opt-in feature that one gets by running --gather-all instead of --stop. The time stamps of the json files still get read and compared against the time --stop is run, i.e. json files appearing while --gather-all runs get ignored (this is unchanged wrt. master or i-ky's branch). With respect to the commits that github notified about, I reworked the branch such that i-ky's commit remains in the history (though I had to resolve a rebase conflict in the readme file).

I hope I don't come across as going over i-ky's head or taking their work away from them, thanks for starting this feature actually!

aras-p commented 3 years ago

I've merged #47 which instead of --start & --stop adds an --all option instead. That sounds like it achieves the same as what this PR would do, so I'm going to close this.