100843 - "xpidl -m typelib -o foo.xpt" generates "foo.xpt.xpt"

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Chad Austin]

This is pretty messy from a build system perspective, in that you have to guess 
the output filename yourself, rather than specifying it explicitly.

I'd go as far as say that 'xpidl -m typelib -o foo' would generate foo instead 
of foo.xpt, but merely checking for redundant extensions would be good enough 
for me for now.

Comment 1

[David Bradley]

I've seen it both ways, default extension or explict. I think leaving it
explicit is the best and easiest approach. Unfortunately it has the danger of
potentially breaking things in the build that might rely on the default
extension. I don't know if anything relies on the default extension or not.

So I agree, checking for redundant extensions is probably the best practical fix
at this time.

Since you assigned this to yourself are you going to make the patch?

Comment 2

[Chad Austin]

When my build finishes I'll start a patch.  ^_^

Long-term, would it be a reasonable goal to make everything explicit?  Should 
any of the build folks be in the CC list?

Comment 3

[David Bradley]

Yes, I'm fine with it being explicit in the long run. I think the logic would be
simpler and cause less confusion for users of the utility. Yes, I'd add some
build folks in case they have opinions or concerns.

I would offer that you could make it explicit and then try building to see what
pops up, since that's an easier change. But my fear is that this might rear it's
head on the Mac, so unless you could also build on the Mac as well, I'd be a
little apprehensive of making it explicit without seeing it build on at least
Win32, Linux, AND Mac.
One last thing, to make this consistent I'd suggest checking the other -m types,
and doing the same, header files, docs, etc. I imagine they all dupe the extension.

And thanks for the help.

Comment 4

[Chad Austin]

cc'ing leaf for a build system perspective.

Comment 5

[Chad Austin]

Okay, I was told leaf prefers @mozilla.org.  Sorry for the spam.  ^^;

Comment 6

[Chad Austin]

Attachment: diff -u

Comment 7

[Chad Austin]

Attachment: explicit output (diff -u)

Comment 8

[Chad Austin]

Attachment: remove redundant extensions (diff -u)

Comment 9

[Chad Austin]

Minor bug in the first patch.  dbradley:  Want to review?  Would shaver be the 
right guy to ask for sr?

Comment 10

[Mike Shaver (:shaver emeritus)]

Well, foo.xpt.xpt is a perfectly legal (though odd) filename on many systems. 
Let's do what I should have done in 1999 and make it explicit for all modes.  (I
wanted cross-mode symmetry for the arguments, but I was obviously stupid.)

The build system effects should be very minimal: we only call xpidl through
rules.mk and the like, AFAICT.

Comment 11

[John Bandhauer]

I hate to be the voice of negativity (again), but..

> The build system effects should be very minimal...

In *our* build system. It is unknown who else might be doing their own thing 
with the tool. They'd all have to suffer the pain of discovery and adjustment 
to the change. I expect Chad wouldn't have openned the bug in the first place if 
he was just letting our build system call it for him. 

I think if you're going to do this then you should be adding an *additional* 
flag for explicitly named generated files and leaving the existing behaviour in 
as is for backward compatibility.

Comment 12

[Chad Austin]

OS/Platform -> All/All

Comment 13

[Chad Austin]

So...  What should I do?  Can we check in the 'remove redundant extensions' 
patch?  Do you want me to work on an 'explicit filename' option?

Personally, I think we could get away with making the change to the build 
system, as long as we let everybody who is using XPCOM know (post to the 
newsgroup?).  We haven't gotten to XPCOM 1.0 yet, so if it is ever going to have 
the Right Behavior, then the switch should happen ASAP.

Comment 14

[cls]

Well, if xpidl isn't frozen, then I don't see the problem with changing to be
explicit as long as all of the callers are changed and people (note in m.xpcom &
m.builds) are informed.  I think this change would be the least of people's
problems when using xpcom. I don't think we should even bother with the "remove
duplicate extensions" change.  

Comment 15

[Chad Austin]

Attachment: explicit output name + Win32 build system patch

Comment 16

[Chad Austin]

I changed rules.mk too, but I haven't tested a build in Linux.  dbradley:  
Wanna try for me?  :)  Somebody also should test on Mac.

Comment 17

[Chad Austin]

cc'ing Simon for Mac build stuff.

Comment 18

[Chad Austin]

bbaetz informs me that the latest patch appears to work in Linux.

He also pointed out that ModeData::suffix could go away, since we always use an 
explicit output filename.  On the other hand, it conveys information about the 
preferred output extension.  Comments?

http://mozilla.org/scriptable/xpidl/index.html should be updated as well.

Comment 19

[Simon Fraser [no longer active]]

beard did the xpt CodeWarrior plugins on Mac.

Comment 20

[John Bandhauer]

I was wondering why no one responded at all to the comment I posted at the very
start of this bug. Now I see that I somehow failed to post the comment at all.

My comment was (in a nutshell)... If it were me, I'd mark this WONTFIX and work
on something that matters.

You all seem so committed to this now. I don't know why.

Comment 21

[John Bandhauer]

And I say again... If you guys really must make the output filename explicit, 
then *please* add a new flag and leave the old behavior. There is no 
justification for breaking other people's build systems in this open source 
project. I will object to such a change.

Comment 22

[Chad Austin]

> My comment was (in a nutshell)... If it were me, I'd mark this WONTFIX and 
> work on something that matters.

I'm gladly doing the work here.  It's a behavior I want to use in XPIDL, and I'm 
contributing the (albeit trivial) fix.

> And I say again... If you guys really must make the output filename explicit, 
> then *please* add a new flag and leave the old behavior. There is no 
> justification for breaking other people's build systems in this open source 
> project. I will object to such a change.

I'm fine with this if the rest of you are.  New patch coming up.

Comment 23

[Chad Austin]

Attachment: -e flag enables explicit output filename

Comment 24

[Chad Austin]

fwiw, we're already "breaking" things with the XP_PC to XP_WIN/XP_WIN32 
switches (bug 56767 and bug 65727).  I don't think a minor build modification 
will affect other projects any more than the rest of the ->1.0 XPCOM changes.

Is there an XPCOM 1.0 specification anywhere?  Any plans in the works?  I see 
in bug 66759 that XPCOM will eventually become a separate library like NSPR, 
which I presume means that it will have its own versioning scheme.

Comment 25

[David Bradley]

Three things:

1. change the description for -e to "use explicit output filename for -o". I was
incorrectly thinking -e was an alternate to -o rather than just modifying the
behavior of -o.

2. I think it should be an error to use -e without the presence of -o, or at
least it should be ignored. Not sure if your logic takes this into account. If I
read it correctly, it would create a file without any extension.

3. I think the change in xpidl_idl.c could be simpler

Could you just add a test up front?

if (explicit_output_filename && !file_basename)
{
    real_outname = g_strdup(outname);
}
else
{
    // original logic here
}

I like factoring of g_strdup_printf calls out, you could leave that in. 

I think having the test early makes it more clear as to what is going on and
eliminates a lot of unnecessary checking in the explicit path.

Comment 26

[John Bandhauer]

Bah. Why not make -e and -o mutually exclusive... Use -e with an explicit name 
OR use -o to set the base name. No reason to make this more complicated to use.

Comment 27

[Chad Austin]

Attachment: incorporate latest suggestions (diff -u)

Comment 28

[Chad Austin]

Attachment: oops! forgot an edge case (-e X -o Y) (diff -u)

Comment 29

[Chad Austin]

r=?  sr=?

Comment 30

[David Bradley]

Comment on attachment 50764 [details] [diff] [review]
oops! forgot an edge case (-e X -o Y) (diff -u)

r=dbradley

Comment 31

[Chad Austin]

shaver?  scc?  anyone?  sr?

Comment 32

[Scott Collins]

Comment on attachment 50764 [details] [diff] [review]
oops! forgot an edge case (-e X -o Y) (diff -u)

prefer pre-increment, where logic allows.  Other than that, sr=scc.

Comment 33

[Chad Austin]

fix checked in

Comment 34

[Phil Schwartau]

Marking Verified -