RFC: Big Makefile patch for WARNS settings

[Ulrich Spörlein-3]

Dear -hackers,

I would like you to give me your thoughts on the attached patch. There
are no functional changes, what I'm trying to do is introduce WARNS?=6
for all top-level Makefiles and override that on a subdir basis.

Why the churn? Because I think it sticks out more, if there's a WARNS=0
in your Makefile rather than the default being WARNS=0. Perhaps this
gets more incentive going for fixing these WARNS. There's also a lot of
work done by the DragonflyBSD folks which I intend to port peu a peu.

Note: There are lots of style changes for games/ and sbin/, which I can
seperate out for easier review. But I'd like to make some sweeping
patches to bring them more inline with style.Makefile(5)

Comments? Committers?
Uli


_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Ed Schouten-3]

Hi Ulrich,

* Ulrich Spörlein <uqs@...> wrote:
> Comments? Committers?

Wouldn't it better to address the root of the problem while there? ;-)

Index: number.c
===================================================================
--- number.c (revision 197852)
+++ number.c (working copy)
@@ -88,9 +88,7 @@
 int lflag;
 
 int
-main(argc, argv)
- int argc;
- char *argv[];
+main(int argc, char *argv[])
 {
  int ch, first;
  char line[256];
@@ -275,7 +273,7 @@
 pfract(len)
  int len;
 {
- static char *pref[] = { "", "ten-", "hundred-" };
+ static char const * const pref[] = { "", "ten-", "hundred-" };
 
  switch(len) {
  case 1:

--
 Ed Schouten <ed@...>
 WWW: http://80386.nl/

[Ulrich Spörlein-3]

On Sun, 11.10.2009 at 19:09:18 +0200, Ed Schouten wrote:
> Hi Ulrich,
>
> * Ulrich Spörlein <uqs@...> wrote:
> > Comments? Committers?
>
> Wouldn't it better to address the root of the problem while there? ;-)

It sure would, but someone[TM] would have to fix all problems for a
top-level dir in a short time frame before new code hits the repo. Or, we
might end up with individual WARNS on 98% of the subdirs and need a big
sweeping patch to then flip the default anyway, so why not now? It
raises the bar for new code entering the tree and we need individual
WARNS lowering anyway due to contrib code.

The only question then is, when do we want to churn. Right now when a
patch is available or do we wait and hope that someday all will be
better?

Anyway, interest seems low for such a big patch so I guess I'll have to
divvy it up for better digestion.

Regards,
Uli
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Gabor Kovesdan-3]

Ed Schouten escribió:
> Hi Ulrich,
>
> * Ulrich Spörlein <uqs@...> wrote:
>  
>> Comments? Committers?
>>    
>
> Wouldn't it better to address the root of the problem while there? ;-)
>  
What I noticed is that the patch sets WARNS?=0 for a lot of utilities,
which actually have higher WARNS-compliance. Even if we don't "address
the root of the problem" right now, it would be nice to elaborate and
set each WARNS level at the higher value possible.For example,
usr.bin/ul is marked as 0 by the patch but it seems to me it is WARNS=6
clean. I don't know the last sources, though, but I'm sure it will
compile with at least WARNS=3.

--
Gabor Kovesdan
FreeBSD Volunteer

EMAIL: gabor@... .:|:. gabor@...
WEB:   http://people.FreeBSD.org/~gabor .:|:. http://kovesdan.org

_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Dag-Erling Smørgrav]

Gabor Kovesdan <gabor@...> writes:
> What I noticed is that the patch sets WARNS?=0 for a lot of utilities,
> which actually have higher WARNS-compliance.

WARNS level 0 is the current default.  All Ulrich's patch does is
reverse the logic so that WARNS is 6 by default and anything that didn't
already set WARNS explicitly sets it to 0, so the actual value of WARNS
in each Makefile is the same as before.  This is orthogonal to actually
fixing whatever doesn't currently build at a higher WARNS level.

DES
--
Dag-Erling Smørgrav - des@...
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Dag-Erling Smørgrav]

Ulrich Spörlein <uqs@...> writes:
> Comments? Committers?

You can set WARNS to 4 for rwhod, since we don't do Alpha any more.

(actually, you can set it to 6, but 4 is what was already there)

DES
--
Dag-Erling Smørgrav - des@...
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Gabor Kovesdan-3]

Dag-Erling Smørgrav escribió:

> Gabor Kovesdan <gabor@...> writes:
>  
>> What I noticed is that the patch sets WARNS?=0 for a lot of utilities,
>> which actually have higher WARNS-compliance.
>>    
>
> WARNS level 0 is the current default.  All Ulrich's patch does is
> reverse the logic so that WARNS is 6 by default and anything that didn't
> already set WARNS explicitly sets it to 0, so the actual value of WARNS
> in each Makefile is the same as before.  This is orthogonal to actually
> fixing whatever doesn't currently build at a higher WARNS level.
>  

Yep, I understand that but what I'm saying is that once we are dealing
with such a big patch, it would be nice to elaborate the highest WARNS
level of each utility and set them accordingly, which doesn't require
too much extra effort as opposed to making all of them WARNS=6 compliant.

--
Gabor Kovesdan
FreeBSD Volunteer

EMAIL: gabor@... .:|:. gabor@...
WEB:   http://people.FreeBSD.org/~gabor .:|:. http://kovesdan.org

_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Dag-Erling Smørgrav]

Gabor Kovesdan <gabor@...> writes:
> Yep, I understand that but what I'm saying is that once we are dealing
> with such a big patch, it would be nice to elaborate the highest WARNS
> level of each utility and set them accordingly, which doesn't require
> too much extra effort as opposed to making all of them WARNS=6
> compliant.

We can do that in a separate patch / commit.

DES
--
Dag-Erling Smørgrav - des@...
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Ulrich Spörlein-3]

On Mon, 12.10.2009 at 12:34:40 +0200, Dag-Erling Smørgrav wrote:
> Ulrich Spörlein <uqs@...> writes:
> > Comments? Committers?
>
> You can set WARNS to 4 for rwhod, since we don't do Alpha any more.
>
> (actually, you can set it to 6, but 4 is what was already there)

Is there some easy way to do cross-compiles (like make universe) in just
one of the subdirs? That would help tremendously.

Bye,
Uli
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Dag-Erling Smørgrav]

Ulrich Spörlein <uqs@...> writes:
> Is there some easy way to do cross-compiles (like make universe) in just
> one of the subdirs? That would help tremendously.

% cd /usr/src
% make toolchain TARGET=powerpc
% make buildenv TARGET=powerpc
%% cd usr.sbin/rwhod
%% make

('make buildenv' starts a subshell)

DES
--
Dag-Erling Smørgrav - des@...
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Doug Barton]

Ulrich Spörlein wrote:
> Dear -hackers,
>
> I would like you to give me your thoughts on the attached patch. There
> are no functional changes, what I'm trying to do is introduce WARNS?=6
> for all top-level Makefiles and override that on a subdir basis.
>
> Why the churn? Because I think it sticks out more, if there's a WARNS=0
> in your Makefile rather than the default being WARNS=0. Perhaps this
> gets more incentive going for fixing these WARNS.

I don't see how this provides an incentive at all.

I also object to this change on the grounds that down the road
debugging is potentially going to be more difficult when someone
forgets that there is a default WARNS level of 6.

One of the strengths of the BSD-style make is the amount of routine
stuff that goes on behind the scenes to make it easier to write good
makefiles. However I don't think this should be one of those things.

> There's also a lot of
> work done by the DragonflyBSD folks which I intend to port peu a peu.

Can you elaborate on this? What work are you planning to port over,
and how does it depend on this default WARNS level issue?

> Note: There are lots of style changes for games/ and sbin/, which I can
> seperate out for easier review. But I'd like to make some sweeping
> patches to bring them more inline with style.Makefile(5)

Putting these two changes in the same patch is not a good thing. It
makes it harder to read diffs, and commits that address separate
issues should be done separately anyway.

That said, I think that the style-compliance issue is a valid one, and
I personally would be in favor of that happening after the
8.0-release, FWIW.


hth,

Doug

--

        Improve the effectiveness of your Internet presence with
        a domain name makeover!    http://SupersetSolutions.com/

_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Ulrich Spörlein-3]

Hi Doug,

On Mon, 12.10.2009 at 16:49:47 -0700, Doug Barton wrote:

> Ulrich Spörlein wrote:
> > Dear -hackers,
> >
> > I would like you to give me your thoughts on the attached patch. There
> > are no functional changes, what I'm trying to do is introduce WARNS?=6
> > for all top-level Makefiles and override that on a subdir basis.
> >
> > Why the churn? Because I think it sticks out more, if there's a WARNS=0
> > in your Makefile rather than the default being WARNS=0. Perhaps this
> > gets more incentive going for fixing these WARNS.
>
> I don't see how this provides an incentive at all.
>
> I also object to this change on the grounds that down the road
> debugging is potentially going to be more difficult when someone
> forgets that there is a default WARNS level of 6.

The "default" would be the setting inherited by, eg,
src/bin/Makefile.inc. This already has a WARNS=6, are you saying that
debugging stuff under bin/ has been made more difficult by that change?
Why do we want bin/ to be WARNS-clean and not care about usr.bin/?

To make things clear, no changes will be made to /usr/share/mk, if
that's what you are afraid if. Unless you do ".include ../Makefile.inc"
somewhere under src/*, you won't get WARNS at all.

> One of the strengths of the BSD-style make is the amount of routine
> stuff that goes on behind the scenes to make it easier to write good
> makefiles. However I don't think this should be one of those things.

One of the strengths of BSD in general that I have come to love is its
higher consistency compared to most other systems. With WARNS=6 under
bin/ and WARNS=2 under sbin/ this consistency is violated.
 
> > There's also a lot of
> > work done by the DragonflyBSD folks which I intend to port peu a peu.
>
> Can you elaborate on this? What work are you planning to port over,
> and how does it depend on this default WARNS level issue?

See
http://gitweb.dragonflybsd.org/dragonfly.git?a=search&h=HEAD&st=commit&s=WARNS6

It depends in no way on the included WARNS level, but "the big switch"
needs to be done anyway, so why not upfront?

> > Note: There are lots of style changes for games/ and sbin/, which I can
> > seperate out for easier review. But I'd like to make some sweeping
> > patches to bring them more inline with style.Makefile(5)
>
> Putting these two changes in the same patch is not a good thing. It
> makes it harder to read diffs, and commits that address separate
> issues should be done separately anyway.
>
> That said, I think that the style-compliance issue is a valid one, and
> I personally would be in favor of that happening after the
> 8.0-release, FWIW.

I will separate those changes out and work some more on them on another
branch. I would really like to get the WARNS changes in first though.

Thanks for all the comments,
Uli
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Doug Barton]

Ulrich Spörlein wrote:
> The "default" would be the setting inherited by, eg,
> src/bin/Makefile.inc. This already has a WARNS=6, are you saying that
> debugging stuff under bin/ has been made more difficult by that change?

It certainly can be, yes. Although admittedly I don't spend a lot of
time debugging stuff under /bin.

> Why do we want bin/ to be WARNS-clean and not care about usr.bin/?

Red herring. I'd like everything to be as warns-clean as possible, I
just disagree that this change will do anything to improve it.

> One of the strengths of BSD in general that I have come to love is its
> higher consistency compared to most other systems. With WARNS=6 under
> bin/ and WARNS=2 under sbin/ this consistency is violated.

The thing that you're glossing over is that most of the stuff in /bin
is our code, and a lot of the stuff in /usr/[s]bin is contrib code.
Thus they actually ARE different. Then of course there is the whole
"Foolish consistency ...." issue.

>>> There's also a lot of
>>> work done by the DragonflyBSD folks which I intend to port peu a peu.
>> Can you elaborate on this? What work are you planning to port over,
>> and how does it depend on this default WARNS level issue?
>
> See
> http://gitweb.dragonflybsd.org/dragonfly.git?a=search&h=HEAD&st=commit&s=WARNS6
>
> It depends in no way on the included WARNS level, but "the big switch"
> needs to be done anyway, so why not upfront?

I disagree with your assertion that "the big switch needs to be done
anyway."

My personal preference would be to see first how many things will need
overrides (WARNS != 6) before deciding whether it's worth setting a
default.


hth,

Doug

--

        Improve the effectiveness of your Internet presence with
        a domain name makeover!    http://SupersetSolutions.com/

_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Ulrich Spörlein-3]

On Mon, 12.10.2009 at 18:37:38 +0200, Dag-Erling Smørgrav wrote:

> Ulrich Spörlein <uqs@...> writes:
> > Is there some easy way to do cross-compiles (like make universe) in just
> > one of the subdirs? That would help tremendously.
>
> % cd /usr/src
> % make toolchain TARGET=powerpc
> % make buildenv TARGET=powerpc
> %% cd usr.sbin/rwhod
> %% make
>
> ('make buildenv' starts a subshell)

Excellent, I now smashed together the following expect script, which can
be used to compile a single subdir with these toolchains. It requires
that you build all toolchains once, upfront.

It's a total hack, but hey it works for me, example usage would be:

$ cd /usr/src
$ universe-build -t
(builds toolchain for all targets, needs to be done every once in a while)
$ universe-build games/grdc WARNS=6 sparc64 amd64 i386
(build grdc with WARNS=6 for 3 archs specified, using the buildenv
target above)

Pardon my weak tcl/expect-fu

Regards,
Uli

#!/usr/local/bin/expect

# Does the following for variable subdirs and a given set of target archs,
# exits upon first failure.

# % cd /usr/src
# % make toolchain TARGET=powerpc
# % make buildenv TARGET=powerpc
# %% cd usr.sbin/rwhod
# %% make

set timeout -1
set toolchain 0

proc usage {argv0}  {
  send_user "usage: $argv0 -t \[target1 target2 ...\]\n"
  send_user "       $argv0 subdir \[make-args\] \[target1 target2 ...\]\n"
  exit
}

while {[llength $argv]>0} {
  set flag [lindex $argv 0]
    switch -- $flag \
      "-t" {
        set toolchain 1
        set argv [lrange $argv 1 end]
        set argc [expr ($argc - 1)]
      } default {
        break
      }
}

if {$toolchain!=1} {
  if {$argc<1} {
    usage $argv0
    exit 1
  }
  set subdir [lindex $argv 0]
  set argv [lrange $argv 1 end]
  set argc [expr ($argc - 1)]
  # if something is present, use it as make args
  if {$argc>=1} {
    set margs [lindex $argv 0]
    set argv [lrange $argv 1 end]
    set argc [expr ($argc - 1)]
  } else {
    set margs {}
  }
}

# if there is still something, use it as target list,
# else default to everything
if {$argc>=1} {
  set targets [lrange $argv 0 end]
} else {
  set input [open "|make universe -V TARGETS" r]
  set targets [split [string trimright [read $input]] " "]
  close $input
}

if {$toolchain==1} {
  foreach target $targets {
    spawn /bin/sh
    expect "$ "
    send "env MAKEOBJDIRPREFIX=/usr/obj/$target make toolchain __MAKE_CONF=/dev/null TARGET=$target\n"
    expect "$ "
    send "exit \$?\n"
    expect eof
  }
  puts "\n"
  exit
}

foreach target $targets {
  spawn /bin/sh
  set sid($target) $spawn_id
  expect "$ "
  send "env MAKEOBJDIRPREFIX=/usr/obj/$target make buildenv __MAKE_CONF=/dev/null TARGET=$target\n"
  expect "$ "
  send "make -C $subdir clean; make -C $subdir $margs\n"
  expect "$ "
  # Stupid buildenv is doing sh || true, so we cannot propagate by doing exit $rc :(
  # grab echo output instead
  send "echo rc=\$?\n"
  expect -re "echo rc=..\r"
  expect -re "rc=(.*)\n"
  # TODO: save rc per target, don't exit below but print rcs for all archs on exit
  set rc $expect_out(1,string)
  send "exit \$?\n"
  expect "$ "
  send "exit \$?\n"
  expect eof
  if {$rc!=0} {
    puts ""
    exit $rc
  }

  ## if we expect eof, the wait below will not return, wtf?
  ## XXX sid($target) wont work here??
  #catch {close -i $spawn_id}
  ## wait returns PID, TYPE, CODE, grab code
  #set rc [lindex [wait sid($target)] 3]
}

_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."