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

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Steve Fink [:sfink] [:s:]]

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.

Comment 1

[Steve Fink [:sfink] [:s:]]

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

Comment 2

[Armen [:armenzg]]

We might also want to avoid needing anything from tools/ and put it under external_tools/ in mozharness.

Comment 3

[Jordan Lund (:jlund)]

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

Comment 4

[Steve Fink [:sfink] [:s:]]

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

Since you rudely rejected my previous attempt, I'll trim things down very slightly and try again.

Comment 5

[Steve Fink [:sfink] [:s:]]

(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 6

[Jordan Lund (:jlund)]

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

Comment 7

[Steve Fink [:sfink] [:s:]]

(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.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b2b7e9f2e56

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b2b7e9f2e56