Closed Bug 1193550 Opened 7 years ago Closed 7 years ago

Add safety check to prevent tools/ dir from getting blown away

Categories

(Release Engineering :: Applications: MozharnessCore, defect)

defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
Tracking Status
firefox43 --- fixed

People

(Reporter: sfink, Assigned: sfink)

Details

Attachments

(1 file, 1 obsolete file)

Terrence ran the spidermonkey_build.py script as documented in https://wiki.mozilla.org/Javascript:Hazard_Builds but he ran it from within his source directory and all hell broke loose. At least one problem was that it checked out the tools repo into tools/, which clobbered the tools/ subdir of his source directory.

That's not a nice thing to do.
We might also want to avoid needing anything from tools/ and put it under external_tools/ in mozharness.
Comment on attachment 8646639 [details] [diff] [review]
Add safety check to prevent tools/ dir from getting blown away

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

I *really* hope this isn't what is needed to prevent tools/ dir from getting blown away :D
Attachment #8646639 - Flags: review?(jlund) → review-
Since you rudely rejected my previous attempt, I'll trim things down very slightly and try again.
Attachment #8647242 - Flags: review?(jlund)
Attachment #8646639 - Attachment is obsolete: true
(In reply to Armen Zambrano Gasparnian [:armenzg] from comment #2)
> We might also want to avoid needing anything from tools/ and put it under
> external_tools/ in mozharness.

Yeah, I'm not using anything special, so that should probably be possible. I don't see much in there right now, though -- maybe it hasn't landed yet? Anyway, it should be safe to land this now since the entire checkout_tools action will get blown away if we switch to external_tools, this change going with it.
Comment on attachment 8647242 [details] [diff] [review]
Add safety check to prevent tools/ dir from getting blown away

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

hrm, taking this one step further, maybe we should add a check that prevents calling a mozharness script from within the work_dir at all. what do you think?

::: testing/mozharness/scripts/spidermonkey_build.py
@@ +350,5 @@
>          dirs = self.query_abs_dirs()
> +
> +        # If running from within a directory also passed as the --source dir,
> +        # this has the danger of clobbering <source>/tools/
> +        if self.config['source']:

I'm not familiar with spidermonkey_build.py but this line assumes 'source' will always be in the config but may have a Falsey value. should this be self.config.get() instead?

@@ +352,5 @@
> +        # If running from within a directory also passed as the --source dir,
> +        # this has the danger of clobbering <source>/tools/
> +        if self.config['source']:
> +            srcdir = self.config['source']
> +            if os.path.samefile(srcdir, os.path.dirname(dirs['abs_tools_dir'])):

oh nice, TIL this and it respects relative paths vs abspaths
Attachment #8647242 - Flags: review?(jlund) → review+
(In reply to Jordan Lund (:jlund) from comment #6)
> Comment on attachment 8647242 [details] [diff] [review]
> Add safety check to prevent tools/ dir from getting blown away
> 
> Review of attachment 8647242 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> hrm, taking this one step further, maybe we should add a check that prevents
> calling a mozharness script from within the work_dir at all. what do you
> think?

So... that's what I did at first, but then I realized that I nearly always use it that way. For compilation, I put my objdir underneath the top source directory (as does the default build), so that I can use hg commands from within my objdir. By extension, I always create a work dir for these analysis runs in the same place, mostly by habit but I could use the same justification.

The "proper" check would be if you're running from directly within any directory that's part of the source checkout. But there's no good way to do that -- you might have the source checked out via mercurial, git, or just an unpacked tarball.

> ::: testing/mozharness/scripts/spidermonkey_build.py
> @@ +350,5 @@
> >          dirs = self.query_abs_dirs()
> > +
> > +        # If running from within a directory also passed as the --source dir,
> > +        # this has the danger of clobbering <source>/tools/
> > +        if self.config['source']:
> 
> I'm not familiar with spidermonkey_build.py but this line assumes 'source'
> will always be in the config but may have a Falsey value. should this be
> self.config.get() instead?

It's a command-line options, and fwict those are always defined. I mimicked the way other command-line options were handled in mozharness.
https://hg.mozilla.org/mozilla-central/rev/5b2b7e9f2e56
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.