On Sun, May 28, 2023 at 04:44:37PM +0200, Omar Polo wrote:
> On 2023/05/28 13:16:34 +0200, Marc Espie <marc.espie.open...@gmail.com> wrote:
> > Turns out that, due to the search rules we use, make
> > is mostly silent in case it couldn't read a Makefile
> >
> > The following patch lets it track the makefilenames that
> > do exist, but that it wasn't able to open, so that
> > a post-mortem can include these.
> >
> >
> > Not sure if other use-cases could also use the post-mortem
> >
> > The tweak to ReadMakefiles is to avoid pushing the same
> > path name twice.
> >
> > Using Lst_Pop ensures this diagnostic only ever occurs once,
> > in case we find other places we might want to stick this.
> >
> > (and realizing that, I should probably add comments in both
> > places instead of explaining this through email)
>
> It would indeed be nice to have a "post-mortem" message with the
> Makefiles that couldn't be opened. it would have saved me some
> headaches in the past with ports that have too tight permissions.
>
> Diff below reads fine to me, just one question inline, but it doesn't
> seem to catch all the situations, consider:
>
> % mkdir /tmp/foo && cd /tmp/foo
> /tmp/foo
> % touch Makefile
> % chmod 000 Makefile
> % make
> make: no target to make.
> % make all
> make: don't know how to make all
> Stop in /tmp/foo
> Makefile(s) that couldn't be read: Makefile
>
> a bare `make' don't print the nice error message, trying with a target
> instead does.
Like I said, yeah, there are more error cases that warrant errors.
We got to be careful, because make has got some default behavior built-in,
so we should only print things if stuff gets erroring out.
> > Index: main.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/make/main.c,v
> > retrieving revision 1.129
> > diff -u -p -r1.129 main.c
> > --- main.c 17 Jan 2023 13:03:22 -0000 1.129
> > +++ main.c 28 May 2023 11:12:48 -0000
> > [...]
> > +static FILE *
> > +open_makefile(const char *fname)
> > +{
> > + FILE *stream;
> > + struct stat buffer;
> > +
> > + stream = fopen(fname, "r");
> > + if (stream != NULL)
> > + return stream;
> > +
> > + if (stat(fname, &buffer) == 0) {
> > + Lst_AtEnd(&unreadable, strdup(fname));
> > + }
>
> Why the stat? Can't we just check errno for EACCES instead? EACCES
> is also returned when search permission is denied for a component of
> the path prefix, which would be a difference but maybe a nice one?
You're right, checking errno makes more sense and would catch more cases.
Actually, the only errno that's "normal" in our case is ENOENT. Anything else
is suspicious.
