westes / flex

The Fast Lexical Analyzer - scanner generator for lexing in C and C++
Other
3.54k stars 529 forks source link

build: Append $(WARNINGFLAGS) to AM_CFLAGS #644

Closed Explorer09 closed 3 months ago

Explorer09 commented 3 months ago

In src/Makefile.am, remove flex_CFLAGS variable, and append $(WARNINGFLAGS) to AM_CFLAGS. The use of flex_CFLAGS has forced Automake to rename flex's object files (.o) to have a 'flex-' prefix. By preventing the rename, the generated src/Makefile.in can be about 37 kB smaller.

Explorer09 commented 3 months ago

@Mightyjo

I like pedantically reiterating the target names in a Makefile that builds two or more programs, libraries, etc.

There are technical disadvantages for the forced name prefixes, considering we are using a Unix make program. In short, it's best to use object file names that are the same as the source files, as it's what the make "suffix rules" are designed for. It's not that we can't have prefixes for object file names, but Automake has to generate a lot of rules for every object file that has name prefix, and the makefile would thus become larger.

Many years ago, it seemed important that stage1flex didn't use the same CFLAGS as flex itself. Reading this now, I think that was a bad call.

It is still true that stage1flex shouldn't use the same CFLAGS as flex, because stage1flex needs a native compiler while flex may be cross compiled. stage1flex should use CFLAGS_FOR_BUILD instead.

And note that I have another branch that I experiment the bootstrap mechanism with, because currently, flex cannot bootstrap on a cross compile setup when EXEEXT differs between --build and --host systems (e.g. Windows / Cygwin and Linux).

Mightyjo commented 3 months ago

Automake has to generate a lot of rules for every object file that has name prefix, and the makefile would thus become larger.

Sure, generating all those rules is Automake's purpose. When maintaining the build system, it's easier to localize problems when the rules are explicit than when they are whatever is implicit in the Make. Since we choose to name a maintainer distribution that provides GNU make for building the distro tarball,

It is still true that stage1flex shouldn't use the same CFLAGS as flex, because stage1flex needs a native compiler while flex may be cross compiled. stage1flex should use CFLAGS_FOR_BUILD instead.

I think your patch makes them both use AM_CFLAGS. I don't see CFLAGS_FOR_BUILD in the stage1flex rules anymore.

And note that I have another branch that I experiment the bootstrap mechanism with, because currently, flex cannot bootstrap on a cross compile setup when EXEEXT differs between --build and --host systems (e.g. Windows / Cygwin and Linux).

Saw that. Bootstrapping is necessary to get an updated distribution tarball. Cross compiling from a distribution tarball doesn't require bootstrapping. So I never saw this as a problem. Just a process you have to do in two steps.

Explorer09 commented 3 months ago

I think your patch makes them both use AM_CFLAGS. I don't see CFLAGS_FOR_BUILD in the stage1flex rules anymore.

Not in this PR. If you are referring to 10025d087899510503059eec4ddd9b624575f5b7 in my repo instead, then yes I try to speed up stage1flex building by reusing object files that are already built for flex.

Mightyjo commented 3 months ago

I hadn't seen your other branch until now. Please don't do that.

With the build system as it is, I can tell whether an object file was built using the most up to date source code generated from our tree. When you re-use objects like that I can't tell how they are influenced by older code or code built into the system's lex.

Your stage2 check might say they're working correctly, but that won't necessarily mean anything. We'd have to prove that Make's own logic is the same as we expect in every build chain.

On Sat, Apr 27, 2024, 20:26 Kang-Che Sung (宋岡哲) @.***> wrote:

I think your patch makes them both use AM_CFLAGS. I don't see CFLAGS_FOR_BUILD in the stage1flex rules anymore.

Not in this PR. If you are referring to 10025d0 https://github.com/westes/flex/commit/10025d087899510503059eec4ddd9b624575f5b7 in my repo instead, then yes I try to speed up stage1flex building by reusing object files that are already built for flex.

— Reply to this email directly, view it on GitHub https://github.com/westes/flex/pull/644#issuecomment-2081263249, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVJXILYEUCIJX6ELI7VCFDY7Q62HAVCNFSM6AAAAABG4GFX2CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBRGI3DGMRUHE . You are receiving this because you were mentioned.Message ID: @.***>

Explorer09 commented 3 months ago

@Mightyjo

I hadn't seen your other branch until now. Please don't do that. With the build system as it is, I can tell whether an object file was built using the most up to date source code generated from our tree. When you re-use objects like that I can't tell how they are influenced by older code or code built into the system's lex. Your stage2 check might say they're working correctly, but that won't necessarily mean anything. We'd have to prove that Make's own logic is the same as we expect in every build chain.

May I argue that you didn't fully understand how make works, did you?

Reusing objects is never a problem. make is supposed to detect source code changes and rebuild them if necessary (by checking file modification dates). What I did is removing steps that the same objects are unnecessary built twice, so the whole build procedure can become faster.

The object files need to be built twice only if the build flags differ -- one using CFLAGS and one using CFLAGS_FOR_BUILD, for example. And Automake rules are also designed with this assumption (object files have prefixed names if they have program-specific CFLAGS to use).

And by the way, the problem I have with that branch (10025d087899510503059eec4ddd9b624575f5b7) isn't whether the bootstrap (for stage1flex) was clean (i.e. no code inherited from the build system's lex), but is that I used a lot of sedding to make it work, and those sed expressions are ugly and difficult to maintain in the long run. I need to re-think of a better approach.

Mightyjo commented 3 months ago

Yes, I understand how make works. Reasonable to ask.

Did you understand why automake best practice is to prefix your flags with your program names? It generates all those rules so we don't have to rely on the version of Make present on the build system. If we rely on Make's implicit rules, we lock ourselves into one mainline (GNU, Solaris, etc.) and create a dependency on that mainline's behavior over time. Automake breaks up both of those dependencies so we remain loosely coupled.

I'm all for reducing build times within the scheme we have.

Mightyjo commented 3 months ago

I should offer how to do what you want in Automake.

Create a no-install library that contains all the sources shared among other targets. Either statically link the new library into your two or more targets or make them depend on the no-install library and cherry pick the objects to link.

Explorer09 commented 3 months ago

@Mightyjo

It generates all those rules so we don't have to rely on the version of Make present on the build system. If we rely on Make's implicit rules, we lock ourselves into one mainline (GNU, Solaris, etc.) and create a dependency on that mainline's behavior over time.

Sorry but I have to correct you. Automake does not rely on the implicit rules of the build system's make program. Automake generates "suffix rules" on its own that override the default rules of the build system's make.

Look in the Makefile.in that Automake generates, and you would find these lines:

COMPILE = $(CC) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) \
$(CPPFLAGS) $(AM_CFLAGS) $(CFLAGS)

.c.o:
@am__fastdepCC_TRUE@    $(AM_V_CC)depbase=`echo $@ | sed 's|[^/]*$$|$(DEPDIR)/&|;s|\.o$$||'`;\
@am__fastdepCC_TRUE@    $(COMPILE) -MT $@ -MD -MP -MF $$depbase.Tpo -c -o $@ $< &&\
@am__fastdepCC_TRUE@    $(am__mv) $$depbase.Tpo $$depbase.Po
@AMDEP_TRUE@@am__fastdepCC_FALSE@   $(AM_V_CC)source='$<' object='$@' libtool=no @AMDEPBACKSLASH@
@AMDEP_TRUE@@am__fastdepCC_FALSE@   DEPDIR=$(DEPDIR) $(CCDEPMODE) $(depcomp) @AMDEPBACKSLASH@
@am__fastdepCC_FALSE@   $(AM_V_CC@am__nodep@)$(COMPILE) -c -o $@ $<

So the object file names without prefix is never a problem. And so...

Did you understand why automake best practice is to prefix your flags with your program names?

There is no such "best practice" as far as I can tell.

I should offer how to do what you want in Automake.

Create a no-install library that contains all the sources shared among other targets. Either statically link the new library into your two or more targets or make them depend on the no-install library and cherry pick the objects to link.

No, it won't work. I have to address the case where stage1flex is natively compiled while flex is cross-compiled. No object code can be shared in this scenario (you are proposing what's similar to @jannick0 's approach in #493, which wouldn't work). I need separate object files so they won't clash:

main.o: compiled from main.c using $(CC), which may be a cross compiler
stage1flex-main.o: compiled from main.c using $(CC_FOR_BUILD), which must be a native compiler
Mightyjo commented 3 months ago

@Mightyjo

It generates all those rules so we don't have to rely on the version of Make present on the build system. If we rely on Make's implicit rules, we lock ourselves into one mainline (GNU, Solaris, etc.) and create a dependency on that mainline's behavior over time.

Sorry but I have to correct you. Automake does not rely on the implicit rules of the build system's make program. Automake generates "suffix rules" on its own that override the default rules of the build system's make.

No correction needed. I think we're saying the same thing. I think we have different views on how much we should rely on Automake v. Make. I prefer relying on automake to build explicit makefiles with lots of prefixes because that makes the build system easier to debug years down the road. I respect your wish to make the build faster and take up less space.

So the object file names without prefix is never a problem. And so...

Did you understand why automake best practice is to prefix your flags with your program names?

There is no such "best practice" as far as I can tell.

This is how the Autotools manuals suggest structuring projects with multiple targets. Makes debugging libtool misbehavior much easier.

I should offer how to do what you want in Automake. Create a no-install library that contains all the sources shared among other targets. Either statically link the new library into your two or more targets or make them depend on the no-install library and cherry pick the objects to link.

No, it won't work. I have to address the case where stage1flex is natively compiled while flex is cross-compiled. No object code can be shared in this scenario (you are proposing what's similar to @jannick0 's approach in #493, which wouldn't work). I need separate object files so they won't clash:

Don't try to bootstrap and cross compile from maintainer sources at the same time. Do it like this:

  1. Bootstrap with make distcheck
  2. Unpack the distribution tarball to a new source tree.
  3. Configure the source tree from the distribution tarball for cross-compiling.
  4. Build from the cross-configured source tree.
Explorer09 commented 3 months ago

@Mightyjo

No correction needed. I think we're saying the same thing.

I don't think so. And it seems like our positions are different. Let me explain…

I think we have different views on how much we should rely on Automake v. Make. I prefer relying on automake to build explicit makefiles with lots of prefixes because that makes the build system easier to debug years down the road. I respect your wish to make the build faster and take up less space.

No, my priority is correctness first, then speed (or code simplicity). As for Automake reliance, the current flex codebase has to rely on Automake anyway, and I didn't change that.

Your claim about "explicit makefiles with lots of prefixes [making] the build system easier to debug years down the road" is simply false. The rules generated file-by-file by Automake as well as suffix rules are all "explicit" enough for flex's uses. If we define "explicit" as "no reliance on default rules of a build system's make but relying on Automake rules is okay", then both approaches are explicit.

And because make prints every command line being executed (unless -s is specified to make), there is no difficulty in debugging the build procedures.

This is how the Autotools manuals suggest structuring projects with multiple targets. Makes debugging libtool misbehavior much easier.

[citation needed].

Don't try to bootstrap and cross compile from maintainer sources at the same time. Do it like this:

1. Bootstrap with `make distcheck`

2. Unpack the distribution tarball to a new source tree.

3. Configure the source tree from the distribution tarball for cross-compiling.

4. Build from the cross-configured source tree.

When @westes added the bootstrap procedure in flex, he did intend the bootstrap procedure to be available in cross compiling as well. There is one use case where you want to bootstrap in a cross-compiled flex program: to build with custom LFLAGS for flex's own scanner. I knew all these can be worked-around when cross-compiling by configuring and building the flex package twice. But that wasn't the procedure that @westes had in mind. (I was one of the person that was against adding the bootstrap procedure in flex's makefile, but that was years ago discussions.)

Mightyjo commented 3 months ago

@Explorer09

When @westes added the bootstrap procedure in flex, he did intend the bootstrap procedure to be available in cross compiling as well. There is one use case where you want to bootstrap in a cross-compiled flex program: to build with custom LFLAGS for flex's own scanner. I knew all these can be worked-around when cross-compiling by configuring and building the flex package twice. But that wasn't the procedure that @westes had in mind. (I was one of the person that was against adding the bootstrap procedure in flex's makefile, but that was years ago discussions.)

I wrote the bootstrap procedure. It's #20. I know what we hand in mind when I wrote it, thanks.

I'm done arguing with you.

Explorer09 commented 3 months ago

I wrote the bootstrap procedure. It's #20. I know what we hand in mind when I wrote it, thanks.

I'm done arguing with you.

I didn't get what arguments you are trying to give to me, but I would mention #109 for the past discussions about bootstrapping and cross-compiling (it's #153 that got merged which fixed the stage1flex during a cross-compile configuration).

And note that the --disable-bootstrap configure option was proposed by me (#129), so users (package builders) can go back to the no-bootstrap build if automated bootstrap build doesn't work (which can happen when flex is cross-compiled).

Mightyjo commented 3 months ago

This is how the Autotools manuals suggest structuring projects with multiple targets. Makes debugging libtool misbehavior much easier.

[citation needed].

For reference: Libtool Manual section 5.3: Using Automake with Libtool Autotools Manual section 8.3: Building a Shared Library

If the taball built by make distcheck needs bootstrapping, then the build system is broken. The bootstrapped sources are supposed to be included in it. I'll take a look at that.

Explorer09 commented 3 months ago

@Mightyjo

If the taball built by make distcheck needs bootstrapping, then the build system is broken. The bootstrapped sources are supposed to be included in it. I'll take a look at that.

You mean this? a7adbf20a91566510f61aa1c3c5193d3ff898d53