If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Make $MOZCONFIG try $topsrcdir-local paths too

RESOLVED WONTFIX

Status

()

Core
Build Config
RESOLVED WONTFIX
11 years ago
6 years ago

People

(Reporter: Håkan Waara, Assigned: hiro)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

11 years ago
I keep a bunch of different mozconfigs in my tree, so each day I manually set the one that I'll use that day. 

For example, in my tree I have mozconfig-firefox-cocoa, mozconfig-firefox-carbon, etc.

70% of the times, I forget that $MOZCONFIG is an absolute path, so I just:

export $MOZCONFIG=mozconfig-firefoz-cocoa , and it won't work. :-)

I'd like mozconfig to try the $topsrcdir to find the mozconfig if it doesn't succeed in the normal ways.
(Reporter)

Comment 1

11 years ago
Created attachment 225253 [details] [diff] [review]
Proposed fix
Attachment #225253 - Flags: review?
(Reporter)

Updated

11 years ago
Attachment #225253 - Flags: review? → review?(benjamin)
(Reporter)

Updated

11 years ago
Attachment #225253 - Flags: review?(benjamin) → review?(mark)
(Reporter)

Comment 2

11 years ago
Comment on attachment 225253 [details] [diff] [review]
Proposed fix

Clearing review request.

Mento wanted me to take some other (more difficult) approach that I don't remember.
Attachment #225253 - Attachment is obsolete: true
Attachment #225253 - Flags: review?(mark)

Comment 3

8 years ago
This bug is a major nuisance since not finding MOZCONFIG means that mk options like OBJDIR are not found, resulting in a source tree build. There are bugs with distclean that mean that fixing a source tree after it gets polluted by object and makefiles is not entirely straightforward. If this is as easy to fix as Hakan suggests, I can't see any obvious downside.

Hakan, do you still not remember why your fix was not acceptable. Can anyone else enlighten?
Created attachment 547591 [details] [diff] [review]
A slightly better solution

Make MOZCONFIG and MY_MOZCONFIG absolute path if the path is relative.

Actually this fix is not a perfect because it is effective only if the current directory is topsrcdir. 

e.g)

In topsrcdir, 

MOZCONFIG=./.mozconfig.local make -f client.mk build

It's OK.

But in the parent directory of topsrcdir,

MOZCONFIG=./.mozconfig.local make -f mozilla-central/client.mk build

It does not work.

But I think it is not a problem for now because the client.mk does not supposed to be worked except on topsrcdir.
Attachment #547591 - Flags: review?(ted.mielczarek)
client.mk should be able to correctly pass $topsrcdir to mozconfig-find:
http://mxr.mozilla.org/mozilla-central/source/client.mk#87

so this should work either way.
Comment on attachment 547591 [details] [diff] [review]
A slightly better solution

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

::: build/autoconf/mozconfig-find
@@ +54,5 @@
> +  fi
> +}
> +
> +[ -n "$MOZCONFIG" ] && MOZCONFIG=`absolute_path "$MOZCONFIG"`
> +[ -n "$MOZ_MYCONFIG" ] && MOZ_MYCONFIG=`absolute_path "$MOZ_MYCONFIG"`

I think you should just remove all references to MOZ_MYCONFIG while you're here. I don't see why anyone would use that.
Attachment #547591 - Flags: review?(ted.mielczarek) → review+
Created attachment 548735 [details] [diff] [review]
Revised patch

Ted, I am sorry that the previous patch does not work well if user specify 
MOZCONFIG as absolute path. I supposed I had tested, but I usually make lots of mistakes. So I did mistake something at that time.

Anyway this new patch removes MOZ_MYCONFIG and use '%%' to check whether the path is absolute or not.

This time I confirmed the following one liners.

In case of relative path:

 MOZCONFIG=.mozconfig-test sh -c 'touch $MOZCONFIG && PWD=`pwd` && FOUND=`./build/autoconf/mozconfig-find $PWD` && [ $FOUND = $PWD/$MOZCONFIG ] && echo 'OK'; rm $MOZCONFIG' 

In case of absolute path:
 MOZCONFIG=/tmp/.mozconfig-test sh -c 'touch $MOZCONFIG && FOUND=`./build/autoconf/mozconfig-find /tmp` && [ $FOUND = $MOZCONFIG ] && echo 'OK'; rm $MOZCONFIG'
Assignee: nobody → hiikezoe
Attachment #547591 - Attachment is obsolete: true
Attachment #548735 - Flags: review?(ted.mielczarek)
Attachment #548735 - Flags: review?(ted.mielczarek) → review+
Keywords: checkin-needed

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [inbound]

Comment 8

6 years ago
This landed as:
http://hg.mozilla.org/mozilla-central/rev/4d794b0ef38d

However, this change causes the following error on win32 using relative configure/pymake:

.././build/autoconf/mozconfig2configure: line 118: Specified MOZCONFIG "../C:\Users\Ed\.mozconfig" does not exist!: No such file or directory

Backing this out locally fixes my existing mozconfig.
OS: Mac OS X → All
Hardware: PowerPC → All
this broke how I build as well, which is to have my mozconfigs in my objdir and a MOZCONFIG=./mozconfig env var.
(In reply to comment #9)
> this broke how I build as well, which is to have my mozconfigs in my objdir
> and a MOZCONFIG=./mozconfig env var.

Is the build for Android?
If it helps, my mozconfig is specified using absolute path via system environment variable $MOZCONFIG=C:\Users\Ed\.mozconfig
talked to hiro on #developers and agreed to back out http://hg.mozilla.org/mozilla-central/rev/a43c3080f472
(In reply to comment #10)
> (In reply to comment #9)
> > this broke how I build as well, which is to have my mozconfigs in my objdir
> > and a MOZCONFIG=./mozconfig env var.
> 
> Is the build for Android?

It is, but I'm also broken for x86 builds
(In reply to comment #9)
> this broke how I build as well, which is to have my mozconfigs in my objdir
> and a MOZCONFIG=./mozconfig env var.

FWIW, I think we might break this use case in the future. If we're going to support relative $MOZCONFIG pathnames, they're going to be srcdir-relative.
(In reply to comment #14)
> (In reply to comment #9)
> > this broke how I build as well, which is to have my mozconfigs in my objdir
> > and a MOZCONFIG=./mozconfig env var.
> 
> FWIW, I think we might break this use case in the future. If we're going to
> support relative $MOZCONFIG pathnames, they're going to be srcdir-relative.

that doesn't make much sense. The mozconfig has the configurations for that build, so specific to the objdir.
Created attachment 550282 [details] [diff] [review]
Support msys path too

This patch supports full path on msys environment. e.g.

C:\Users\Ed\.mozconfig
/c/Users/Ed/.mozconfig
\\YourHost\.mozconfig
Attachment #548735 - Attachment is obsolete: true
Attachment #550282 - Flags: review?(ted.mielczarek)
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #9)
> > > this broke how I build as well, which is to have my mozconfigs in my objdir
> > > and a MOZCONFIG=./mozconfig env var.
> > 
> > FWIW, I think we might break this use case in the future. If we're going to
> > support relative $MOZCONFIG pathnames, they're going to be srcdir-relative.
> 
> that doesn't make much sense. The mozconfig has the configurations for that
> build, so specific to the objdir.

But the current directory at that time is topsrcdir? If so , it is natual to use MOZCONFIG=objdir/mozconfig.
(In reply to comment #17)
> But the current directory at that time is topsrcdir? If so , it is natual to
> use MOZCONFIG=objdir/mozconfig.
I don't follow. My point is that the mozconfig is associated with the build, so it should be relative to the objdir, not the topsrcdir.
Blassey, actually the mozconfig is associated with the _run_ we are on. Since note client.mk uses it.

*and* we have always supported |mk_add_option MOZ_OBJDIR=FOO| and one common foo there is

|mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-@CONFIG_GUESS@| which is mentioned on the docs at MDC

If someone can specify relative dir's, the two spots this would get invoked from would differ in where to find it. (We can't relatively search for a file from something that the file itself can identify a location for)

So I'd say that relatively going from objdir is a no-go. As useful as it feels in theory.
cf bug 675691 for where I think we should be going.
Comment on attachment 550282 [details] [diff] [review]
Support msys path too

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

I hate shell scripting. :-(
Attachment #550282 - Flags: review?(ted.mielczarek) → review+
Keywords: checkin-needed
Whiteboard: [inbound]
http://hg.mozilla.org/integration/mozilla-inbound/rev/2b05e85795d1
Keywords: checkin-needed
Whiteboard: [inbound]
Backed-out from inbound due to a perma-orange, see:
http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0adaae140642

The patch in this bug might not bee related, the entire push has been backed out.
Whiteboard: [inbound]
Created attachment 551773 [details] [diff] [review]
patch

That patch still breaks me. Its not clear what this change improves, but if you're going to do it, how about you not break my build?
Attachment #550282 - Attachment is obsolete: true
Attachment #551773 - Flags: review?(ted.mielczarek)
(In reply to Brad Lassey [:blassey][blassey@mozilla.com] from comment #24)
> Created attachment 551773 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> That patch still breaks me. Its not clear what this change improves, but if
> you're going to do it, how about you not break my build?

If we can use a magic keyword which represents $top_buildir (i.e. MOZ_OBJDIR) in shell, it must be a solution for you.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> If we can use a magic keyword which represents $top_buildir (i.e.
> MOZ_OBJDIR) in shell, it must be a solution for you.
The patch I attached works
Just out of curiosity, what is your MOZCONFIG when fresh building? You make a MOZ_OBJDIR and create a .mozconfig in there first?
yup, I do:
$> mkdir objdir-droid
$> emacs objdir-droid/mozconfig
(In reply to Brad Lassey [:blassey][blassey@mozilla.com] from comment #28)
> yup, I do:
> $> mkdir objdir-droid
> $> emacs objdir-droid/mozconfig

Thanks.

IMHO, these steps before building and set env value (in this case it's MOZCONFIG) relative to target (building) directory are weird at least for me (Especially second one). But I leave the decision up to other developers.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> IMHO, these steps before building and set env value (in this case it's
> MOZCONFIG) relative to target (building) directory are weird at least for me
> (Especially second one). But I leave the decision up to other developers.

That's fine, you don't have to build that way. But why break it when you don't have to?

Also, as I said in comment 24, I don't see what value this bug/patch adds. What problem are you trying to fix?
(In reply to Brad Lassey [:blassey][blassey@mozilla.com] from comment #30)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> > IMHO, these steps before building and set env value (in this case it's
> > MOZCONFIG) relative to target (building) directory are weird at least for me
> > (Especially second one). But I leave the decision up to other developers.
> 
> That's fine, you don't have to build that way. But why break it when you
> don't have to?
> 
> Also, as I said in comment 24, I don't see what value this bug/patch adds.
> What problem are you trying to fix?

I feel relative to objdir is counter to what we were/are doing here now, and counter to what we have always recommended/supported. I'll let ted make the call though, the original patch here *had* review, which is why I landed it. But I'm surely not module-owner.
I just uploaded a patch to bug 675691 that goes in the other direction, removing , btw. I think this bug should be wontfix'd (with my reasoning in bug 675691 comment 0).
> removing , 

er, I meant "removing things to guess".
Comment on attachment 551773 [details] [diff] [review]
patch

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

I don't think we're going to take this. I actually think we're going to take the patch in bug 675691 instead.
Attachment #551773 - Flags: review?(ted.mielczarek) → review-

Updated

6 years ago
Blocks: 682897
I believe we're not going to do this.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.