Closed Bug 1024471 Opened 7 years ago Closed 7 years ago

Make run/debug commands use a scratch profile by default

Categories

(Firefox Build System :: Mach Core, enhancement)

x86_64
Linux
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: jdm, Assigned: philippe.chassagnard)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

I think that requiring people to add a profile argument is a gotcha that we can avoid, and we can make the experience for first-time users nicer. My argument is that in general, preserving data between testing sessions is not a necessity, and it's easier to teach people how to use an explicit profile later than it is to explain to new users why they're having trouble running a dev build at the same time as their regular Firefox instance. I'd like to make ./mach run/debug without any profile argument created a scratch profile dir in /tmp and run with that.
Yes, let's do this. As long as we have an optional profile argument so you can pass in an existing profile, creating a fresh profile for each run by default sounds like a great idea.
As much as I support this, I suspect there will be objection to it. I really wish we had proper config files for mach to make this kind of choice configurable. Config files go a long way towards silencing the mob.

That being said, I will r+ a patch that changes this as long as we can specify the profile via extra arguments.
I have to say, I would expect the people who care about this behaviour to already be using the -P argument explicitly, rather than having set the default profile for Firefox and depending on that being used when -P is not present.
Why not create a scratch profile in $objdir, that would be clobbered by a clobber, but kept otherwise?
The main reason _I_ need a scratch profile is not to have it cleaned up every time, but, simply, to have it *not* being my firefox profile, which I'm running firefox on anyways. I guess most people are in this case.
(In reply to comment #4)
> Why not create a scratch profile in $objdir, that would be clobbered by a
> clobber, but kept otherwise?
> The main reason _I_ need a scratch profile is not to have it cleaned up every
> time, but, simply, to have it *not* being my firefox profile, which I'm running
> firefox on anyways. I guess most people are in this case.

Yes please!
Made a mistake in my comment. The default user is made in objdir/tmp/scratch_user
Attachment #8444052 - Flags: review?(gps)
Comment on attachment 8444052 [details] [diff] [review]
Default user is now created in objdir/tmp/scratch_user

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

Please change the title in the commit message to objdir/tmp/scratch_user to avoid mislead.
Attachment #8444052 - Attachment description: Default user is now created in /tmp/scratch_user → Default user is now created in objdir/tmp/scratch_user
Comment on attachment 8444052 [details] [diff] [review]
Default user is now created in objdir/tmp/scratch_user

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

I could bikeshed about the name and path of the profile directory. But I think this is fine.

We really need config file support for mach so people can customize the default behavior. Not your problem to solve.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +751,5 @@
> +        if '-profile' not in params and '-P' not in params:
> +            if not os.path.isdir(self.topobjdir + "/tmp/scratch_user"):
> +                os.makedirs(self.topobjdir + "/tmp/scratch_user")
> +            args.append('-profile')
> +            args.append(self.topobjdir + "/tmp/scratch_user")

Does the forward slash work on Windows?

Also, please using os.path.join() or mozpack.path.join() (the latter always uses / as opposed to platform-dependent) for joining filenames. It's how you do path manipulation in python, for better or worse.

@@ +845,5 @@
> +        if '-profile' not in params and '-P' not in params:
> +            if not os.path.isdir(self.topobjdir + "/tmp/scratch_user"):
> +                os.makedirs(self.topobjdir + "/tmp/scratch_user")
> +            args.append('--profile')
> +            args.append(self.topobjdir + "/tmp/scratch_user")

Seeing this redundant block has me wondering how different debug() and run() actually are. It seems like one should be implemented in terms of the other. Not your problem to fix though.
Comment on attachment 8444052 [details] [diff] [review]
Default user is now created in objdir/tmp/scratch_user

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

r+ on condition of fixing commit message and using appropriate path joining.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +751,5 @@
> +        if '-profile' not in params and '-P' not in params:
> +            if not os.path.isdir(self.topobjdir + "/tmp/scratch_user"):
> +                os.makedirs(self.topobjdir + "/tmp/scratch_user")
> +            args.append('-profile')
> +            args.append(self.topobjdir + "/tmp/scratch_user")

Does the forward slash work on Windows?

Also, please using os.path.join() or mozpack.path.join() (the latter always uses / as opposed to platform-dependent) for joining filenames. It's how you do path manipulation in python, for better or worse.

@@ +845,5 @@
> +        if '-profile' not in params and '-P' not in params:
> +            if not os.path.isdir(self.topobjdir + "/tmp/scratch_user"):
> +                os.makedirs(self.topobjdir + "/tmp/scratch_user")
> +            args.append('--profile')
> +            args.append(self.topobjdir + "/tmp/scratch_user")

Seeing this redundant block has me wondering how different debug() and run() actually are. It seems like one should be implemented in terms of the other. Not your problem to fix though.
Attachment #8444052 - Flags: review?(gps) → review+
Attachment #8444052 - Attachment is obsolete: true
Attachment #8445831 - Flags: review?(gps)
Attachment #8445831 - Flags: review?(gps) → review+
Assignee: nobody → philippe.chassagnard
Let's get this merged! There's no need for a try run since this isn't used by any automated tests.
Keywords: checkin-needed
This doesn't apply to inbound. Please rebase.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0885143017d0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1035765
Depends on: 1038558
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.