974281 - `mach clobber` clobbers msvc directory (generated visual studio project)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

`mach clobber` shouldn't clobber the generated Visual Studio project files. It takes too long to reindex after a clobber and most of the time it isn't necessary. The generated project should live outside the objdir. I've been using Visual Studio with the solution outside the objdir and the solution never being clobbered for 3 years until now and it seems to work very well this way.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

1. This causes "mach clobber" to fail when you have the solution open in MSVS2013. This causes trouble.

2. When you *do* run "mach clobber && mach build," which is something that is required multiple times per week (sometimes multiple times per day) you cannot do useful work within Visual Studio because the project is not there yet.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

3. Clobbering the generated project and then re-generating it causes indexing to reset from zero. But, indexing from zero is too slow.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Move the generated Visual Studio solution out of the obj directory.

This simple change appears to work for me, without ant issues.

Changed the directory name to .msvc, so it's more obvious that it is not part of the tracked files.

Added to .hgignore and .gitignore.
I've never used either of these before, so hopefully the syntax is correct.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8628220 [details] [diff] [review]
Move the generated Visual Studio solution out of the obj directory.

Review of attachment 8628220 [details] [diff] [review]:
-----------------------------------------------------------------

Ted and I talked about this at Whistler after this question was asked to me and we came up with a different solution but I was unable to follow up because I didn't know who asked the question.

Anyway, the visual studio project files are tied to a specific build configuration. e.g. the project files for debug builds will vary from non-debug builds. If we put the project files in the source directory, this works for workflows that have a single object directory per source directory. However, some people have multiple object directories per source directory and this change would make Visual Studio unusable to them.

The relatively simple solution we came up with was to change the default behavior of `mach clobber` to ignore certain directories in the objdir, starting with "msvc." i.e. `mach clobber` would remove everything except "objdir/msvc." We'd likely want to introduce a `mach clobber --full` or some such to force a delete of the entire objdir.

You can find the `mach clobber` code in python/mozbuild/mozbuild/mach_commands.py. If this is beyond your Python skill level, let me know - I could probably whip it up in a few minutes.

Comment 5

[Bob Owen (:bobowen)]

(In reply to Gregory Szorc [:gps] from comment #4)

> Ted and I talked about this at Whistler after this question was asked to me
> and we came up with a different solution but I was unable to follow up
> because I didn't know who asked the question.

It was Jim Mathies that asked about this.
 
> The relatively simple solution we came up with was to change the default
> behavior of `mach clobber` to ignore certain directories in the objdir,
> starting with "msvc." i.e. `mach clobber` would remove everything except
> "objdir/msvc." We'd likely want to introduce a `mach clobber --full` or some
> such to force a delete of the entire objdir.

How about using?:
  self._out_dir = os.path.join(self.environment.topsrcdir, '.msvc',
                               os.path.relpath(self.environment.topobjdir,
                                               self.environment.topsrcdir))

Then you wouldn't need to change clobber.
 
> You can find the `mach clobber` code in
> python/mozbuild/mozbuild/mach_commands.py. If this is beyond your Python
> skill level, let me know - I could probably whip it up in a few minutes.

I'm no python expert, but I could give it a go, it would be next week though (at least).
It would definitely take me more than a few minutes.

Comment 6

[Gregory Szorc [:gps]]

I'd prefer we not store more state in the source directory. We want to be trending towards the ability to build from a read-only source directory. Also, if you add .msvc, someone else will want to add .eclipse. And we have a slippery slope. This is all build-specific state and IMO should be stored inside the objdir because the objdir is our container for build-specific state.

Comment 7

[Bob Owen (:bobowen)]

Too many other things are piling up, so de-assigning for the moment.

Comment 8

[Bob Owen (:bobowen)]

Attachment: Move the generated Visual Studio solution out of the obj directory.

Here's my patch with the solution moved up a level, but with individual directories to allow for different build configs.

I spoke to Jim about this in passing in Orlando and he felt that it shouldn't go in the obj directory, I think mainly because people use other methods than mach clobber to clear their obj directory.

I'll let Jim chime in with anything else.

Comment 9

[Jim Mathies [:jimm]]

(In reply to Gregory Szorc [:gps] from comment #4)
> The relatively simple solution we came up with was to change the default
> behavior of `mach clobber` to ignore certain directories in the objdir,
> starting with "msvc." i.e. `mach clobber` would remove everything except
> "objdir/msvc." We'd likely want to introduce a `mach clobber --full` or some
> such to force a delete of the entire objdir.

FWIW, I don't use mach clobber because it is very slow. I use a batch script that renames obj folders to backup names and then invokes a series of Windows command line tools to purge the folders quickly. Two advantages to this, the Windows remove directory tools I invoke are substantially faster than anything else available, and I can start building immediately without waiting for delete operations to complete.

Because I use this method, the auto-generated visual studio files are unusuable. If I do use the auto-generated files, every time I clobber (once a week or so on average) I have to shut down vs and lose my project and environmental settings. As a Windows dev who uses vs exclusively I invest a substantial amount of time in setting vs up the way I like it, so this is a deal killer for me. I've stopped using the auto-generated feature and have gone back to maintaining my own Mozilla visual studio project.

I'd like to propose we land Bob's work here. It far exceeds the current solution in terms of usability, and devs shouldn't have much trouble working around the corner case idiosyncrasies.

Comment 10

[Gregory Szorc [:gps]]

We talked about this at a build system work week and decided that we're going to change the behavior of `mach clobber` to clobber everything except a whitelist of directories and the visual studio files will be in that whitelist.

There's a risk that changing to N invocations of rm.exe will slow things down. If that happens, we'll cross that bridge if we come to it.

Speaking of rm.exe, the latest MozillaBuild ships a special rm executable that uses Win32 APIs that delete directories efficiently. When you complain about `mach clobber` being slow, that is surprising to me.

Comment 11

[Jim Mathies [:jimm]]

(In reply to Gregory Szorc [:gps] from comment #10)
> Speaking of rm.exe, the latest MozillaBuild ships a special rm executable
> that uses Win32 APIs that delete directories efficiently. When you complain
> about `mach clobber` being slow, that is surprising to me.

Maybe the remove operation isn't, I haven't compared the timing. The scripts I use don't care about that operation though, they rename the folder and then delete it. These scripts are also batch files so I run and minimize them and start a build immediately. 

here's the batch script for a mc clobber on my system:

move F:\Mozilla\DBG F:\Mozilla\DBG_deltmp
move F:\Mozilla\REL F:\Mozilla\REL_deltmp
del F:\Mozilla\DBG_deltmp /F /Q > nul
del F:\Mozilla\REL_deltmp /F /Q > nul
rmdir F:\Mozilla\DBG_deltmp /S /Q
rmdir F:\Mozilla\REL_deltmp /S /Q

Comment 12

[(not currently active) Ted Mielczarek]

`mach clobber` only got fixed to use winrm ~8 months ago: bug 1162519. It would be interesting if you could test if it's still slow for you.

Comment 13

[Jim Mathies [:jimm]]

I'll let you know. It's not going to as fast as a background delete of a renamed obj dir though. :) But if I need to live with waiting for clobbers to use the project files it's a pretty minor productivity sacrifice for a big productivity win.

Comment 14

[Gregory Szorc [:gps]]

I'll try to hack on this this week.

Comment 15

[Gregory Szorc [:gps]]

Attachment: Bug 974281 - Use mozfile.remove since rmtree is deprecated;

My Python editor told me rmtree has been marked as deprecated. Use
mozfile.remove instead.

Review commit: https://reviewboard.mozilla.org/r/35971/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35971/

Comment 16

[Gregory Szorc [:gps]]

Attachment: Bug 974281 - Do not clobber msvc directory by default;

The "remove_objdir" method has been rewritten to support partial
clobber. It still defaults to full clobber. But the "full" argument
can be passed as False to limit to a partial clobber where currently
the "msvc" directory (contains Visual Studio files) is not clobbered.

On Windows, there might be a regression with this change because
we'll be invoking N winrm.exe processes and new processes have
a non-trivial overhead on Windows. However, it is hopefully unlikely
that new processes are more overhead than deleting hundreds of thousands
of files.

Review commit: https://reviewboard.mozilla.org/r/35973/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35973/

Comment 17

[Jim Mathies [:jimm]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #12)
> `mach clobber` only got fixed to use winrm ~8 months ago: bug 1162519. It
> would be interesting if you could test if it's still slow for you.

eh, not great, not awful. mach clobber of a release obj dir took about 2 min. 15 seconds. My scripts blew away a debug obj dir in about 1 minute 15 seconds.

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 8722291 [details]
Bug 974281 - Use mozfile.remove since rmtree is deprecated;

https://reviewboard.mozilla.org/r/35971/#review35419

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 8722292 [details]
Bug 974281 - Do not clobber msvc directory by default;

https://reviewboard.mozilla.org/r/35973/#review35425

Did you compare timings of this vs. the existing single-invocation? It would be good to at least sanity check that it's not a huge regression.

::: python/mozbuild/mozbuild/base.py:353
(Diff revision 1)
> +                subprocess.check_call(['winrm', '-rf', path])

One fairly simple optimization here would be to run the processes using `subprocess.Popen` instead of `check_call`, and then if we have psutil, use `psutil.wait_procs` to wait for the whole set of them (if we don't have psutil you could just loop over the list and call `wait` on each in turn). This way we'd be running all the rm processes in parallel.

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8722291 [details]
Bug 974281 - Use mozfile.remove since rmtree is deprecated;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35971/diff/1-2/

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8722292 [details]
Bug 974281 - Do not clobber msvc directory by default;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35973/diff/1-2/

Comment 22

[Jim Mathies [:jimm]]

Hey gps, can we get these landed?

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 8722291 [details]
Bug 974281 - Use mozfile.remove since rmtree is deprecated;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35971/diff/2-3/

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8722292 [details]
Bug 974281 - Do not clobber msvc directory by default;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35973/diff/2-3/

Comment 25

[Gregory Szorc [:gps]]

Comment on attachment 8722292 [details]
Bug 974281 - Do not clobber msvc directory by default;

ted: can you please look at this again since the rebase changed things considerably?

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 8722292 [details]
Bug 974281 - Do not clobber msvc directory by default;

https://reviewboard.mozilla.org/r/35973/#review70358

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a09ef1eb1d63
Use mozfile.remove since rmtree is deprecated; r=ted
https://hg.mozilla.org/integration/autoland/rev/c4c26c0af171
Do not clobber msvc directory by default; r=ted

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a09ef1eb1d63
https://hg.mozilla.org/mozilla-central/rev/c4c26c0af171

Comment 29

[Gregory Szorc [:gps]]

This landed. Cancelling needinfo.