configure: improve cross-compilation detection

[mobin-2008]

Assume the build as native if both $CXX and $CXX_FOR_BUILD have the same value. If the compiler for the host and the build machine are the same, it's a native build. There is a chance that the user may set both $CXX and $CXX_FOR_BUILD to the same compiler. For example, cbuild (Chimera Linux package builder) sets both variables regardless of native or cross build and because we use $CXX_FOR_BUILD for detecting cross build, we should unset that variable in this case.

Also, GNU autotools detects this case and assumes the build as native.

[mobin-2008]

I just realised that the whole commit message should be written imperatively, not just the commit title or first sentence of commit comment.

[davmac314]

I just realised that the whole commit message should be written imperatively, not just the commit title or first sentence of commit comment.

No, background information may follow. Your message here really isn't too bad and it's an improvement on previous commit messages. However, it could be improved:

Assume the build as native if both $CXX and $CXX_FOR_BUILD have the same value. If the compiler for the host and the build machine are the same, it's a native build

1. Assume the build is native.
2. These two sentences basically say the same thing; the 2nd one is just rephrasing the first. So when writing you make that clear, eg:

Assume the build is native if both $CXX and $CXX_FOR_BUILD have the same value (i.e. if the compiler for the host and the build machine are the same, it's a native build).

(This is nothing to do with commit messages per se - it's just better writing).

There is a chance that the user may set both $CXX and $CXX_FOR_BUILD to the same compiler

This is basically saying the same thing, a third time - I don't think it's necessary.

For example, cbuild (Chimera Linux package builder) sets both variables regardless of native or cross build and because we use $CXX_FOR_BUILD for detecting cross build, we should unset that variable in this case.

This is background info and in general the style used in commit messages is to start a new paragraph for the background information. You should read through the existing commit messages to get a sense for this. Here are some recent examples:

write_nx: use correct format specifier for unsigned types

In write_nx, use the correct 'printf' format specifier ('%u', '%lu') for
unsigned types.

As written beforehand, the signed and unsigned versions use the same
(signed value) format specifier, so large unsigned values would be
output incorrectly.

And

dinitctl: replace hardcoded name in error messages

Use a macro, DINITCTL_APPNAME, for the application name ("dinitctl") for
use in error messages etc.

This will make it easier to change the name or (with extra work) to use
the name used for invocation (i.e. argv[0]) at a later point.

See the general structure?

we should unset that variable in this case.

This is explaining what the change does at the code level. It doesn't make sense at the abstract level of the commit message (why should we unset the variable? we could also just flag "not a cross-build" internally in some other way).

Also, GNU autotools detects this case and assumes the build as native.

If the point is that the new behaviour is consistent with autotools, then it's better to say that directly rather than force the reader to make that connection themselves (even if it's fairly simple, there's no reason not to state your point explicitly).

[mobin-2008]

Sorry for delay, I got sick. I'm better now.

I just realised that the whole commit message should be written imperatively, not just the commit title or first sentence of commit comment.

No, background information may follow. Your message here really isn't too bad and it's an improvement on previous commit messages. However, it could be improved:

Assume the build as native if both $CXX and $CXX_FOR_BUILD have the same value. If the compiler for the host and the build machine are the same, it's a native build

1. Assume the build _is_ native.

2. These two sentences basically say the same thing; the 2nd one is just rephrasing the first. So when writing you make that clear, eg:

Assume the build is native if both $CXX and $CXX_FOR_BUILD have the same value (i.e. if the compiler for the host and the build machine are the same, it's a native build).

(This is nothing to do with commit messages per se - it's just better writing).

I added the first sentence after my comment. It wasn't there and I added it without thinking twice because I wanted to rewrite it after I set the PR to draft. Thanks for the suggestion. Yes, It's better.

There is a chance that the user may set both $CXX and $CXX_FOR_BUILD to the same compiler

This is basically saying the same thing, a third time - I don't think it's necessary.

For example, cbuild (Chimera Linux package builder) sets both variables regardless of native or cross build and because we use $CXX_FOR_BUILD for detecting cross build, we should unset that variable in this case.

This is background info and in general the style used in commit messages is to start a new paragraph for the background information. You should read through the existing commit messages to get a sense for this. Here are some recent examples:

write_nx: use correct format specifier for unsigned types

In write_nx, use the correct 'printf' format specifier ('%u', '%lu') for
unsigned types.

As written beforehand, the signed and unsigned versions use the same
(signed value) format specifier, so large unsigned values would be
output incorrectly.

And

dinitctl: replace hardcoded name in error messages

Use a macro, DINITCTL_APPNAME, for the application name ("dinitctl") for
use in error messages etc.

This will make it easier to change the name or (with extra work) to use
the name used for invocation (i.e. argv[0]) at a later point.

See the general structure?

Yes. I need to get better in breaking down text to sections to understand them better.

we should unset that variable in this case.

This is explaining what the change does at the code level. It doesn't make sense at the abstract level of the commit message (why should we unset the variable? we could also just flag "not a cross-build" internally in some other way).

1. unset is mentioned in the commit message because I'm explaining why we need to unset and how that fixes the issue.
2. Currently the main purpose of $CXX_FOR_BUILD in the code is to use another compiler for the build machine which is required for cross-compiling. It can be read like this:
   $CXX_FOR_BUILD is set: cross build.
   $CXX_FOR_BUILD is not set: native build.
   I think it's the correct thing to unset that variable at that point when it's a native build. Is there any reason to have special handling for this case?

Also, GNU autotools detects this case and assumes the build as native.

If the point is that the new behaviour is consistent with autotools, then it's better to say that directly rather than force the reader to make that connection themselves (even if it's fairly simple, there's no reason not to state your point explicitly).

Yes. That's better to explicitly state that point.

[davmac314]

1. unset is mentioned in the commit message because I'm explaining why we need to unset and how that fixes the issue.

2. Currently the main purpose of $CXX_FOR_BUILD in the code is to use another compiler for the build machine which is required for cross-compiling. It can be read like this:
   $CXX_FOR_BUILD is set: cross build.
   $CXX_FOR_BUILD is not set: native build.
   I think it's the correct thing to unset that variable at that point when it's a native build. Is there any reason to have special handling for this case?

I think you're misinterpreting what I said. I'm not saying the solution is wrong.

I think it's the correct thing to unset that variable at that point when it's a native build.

That's ok as a solution, but, I think the comment doesn't explain that very well. It doesn't say where the change is, it doesn't really explain how the solution works; you have to look at the code to make sense of it.

[davmac314]

To explain more:

I'm explaining why we need to unset and how that fixes the issue

We don't need to unset the variable. That's the solution you have chosen.

And, it didn't explain how that fixes the issue. What was written was:

because we use $CXX_FOR_BUILD for detecting cross build, we should unset that variable in this case

it doesn't say how we "use" $CXX_FOR_BUILD for detecting a cross build, and it doesn't say how/why unsetting the variable alters the result of that detection. For example it doesn't explain the variable is unset just before the check for whether the $CXX_FOR_BUILD is set or not (which is used to detect a cross build). That's so easy to say and makes it so much clearer.

[mobin-2008]

I'm still alive and I will fix the mentioned things once the situation gets better.

[davmac314]

I'm still alive and I will fix the mentioned things once the situation gets better.

No worries, Mobin. I'm aware of the situation. Stay safe.

[mobin-2008]

I cleaned up both commit message and code comments explaining the change, to be more clear and to respect the context of each other. The commit message doesn't mention variables anymore because it doesn't make sense to talk about it in the context of commit message. In the other hand, code comments now focus on variables which make sense in the context of the code.

Also the commit message is separated into three sections to be consistent with style of other commits.

[davmac314]

Hey Mobin, welcome back!

The commit message is better, thanks. I do still have a few minor concerns about this PR:

• The commit message still refers to "broken cross-compilation detection", that's unnecessarily negative and not really correct; the current cross-compilation detection isn't broken - it works exactly as it was designed to! You could phrase it in terms of improvement or nature of the change.

• Relying on $CXX_FOR_BUILD == $CXX as an indicator of native build seems problematic, what if the same compiler is used for both host and target builds? For example clang can target a different platform via --target= which could be specified in CXXFLAGS_FOR_BUILD (and LDFLAGS_FOR_BUILD), with CXX and CXX_FOR_BUILD both set to clang++. If this was done, the current behaviour would be to assume a cross-build (with unknown target platform) and issue a warning. The new behaviour (introduced by this PR) would be to silently, and incorrectly, assume a native build. How is the new behavior better? The new behaviour seems worse to me, in this case at least.

• The comment in the configure code reads as if it was a commit message. It's not really worth talking about cbuild or autotools; just explain what the code is doing, and/or why at the level of the code itself, not at the level of a broad overview. It's also confusing:

  Unset $CXX_FOR_BUILD now to be consistent with GNU autotools and what people expect.

  But this is only done if $CXX and $CXX_FOR_BUILD have the same value. This sentence in the comment makes it seem like it's done unconditionally; the sentence is also part of a paragraph which is giving background info, which means it's in the wrong place (should be a new paragraph, it's a new subject).

[mobin-2008]

Hey Mobin, welcome back!

Thanks! I missed the project so much.

The commit message is better, thanks. I do still have a few minor concerns about this PR:

• The commit message still refers to "broken cross-compilation detection", that's unnecessarily negative and not really correct; the current cross-compilation detection isn't broken - it works exactly as it was designed to! You could phrase it in terms of improvement or nature of the change.

OK.

• Relying on $CXX_FOR_BUILD == $CXX as an indicator of native build seems problematic, what if the same compiler is used for both host and target builds? For example clang can target a different platform via --target= which could be specified in CXXFLAGS_FOR_BUILD (and LDFLAGS_FOR_BUILD), with CXX and CXX_FOR_BUILD both set to clang++. If this was done, the current behaviour would be to assume a cross-build (with unknown target platform) and issue a warning. The new behaviour (introduced by this PR) would be to silently, and incorrectly, assume a native build. How is the new behavior better? The new behaviour seems worse to me, in this case at least.

I didn't account for that. I think the problem here is that it should also check for *_FOR_BUILD variables because they affect the build environment and make a difference between host and build machine. I think this addresses your concern about the new behaviour. I try to think about all scenarios that make this check fail to correctly identify a native-build if there are any except the case you mentioned.

• The comment in the configure code reads as if it was a commit message. It's not really worth talking about cbuild or autotools; just explain what the code is doing, and/or why at the level of the code itself, not at the level of a broad overview. It's also confusing:

Unset $CXX_FOR_BUILD now to be consistent with GNU autotools and what people expect.

But this is only done if $CXX and $CXX_FOR_BUILD have the same value. This sentence in the comment makes it seem like it's done unconditionally; the sentence is also part of a paragraph which is giving background info, which means it's in the wrong place (should be a new paragraph, it's a new subject).

I will rewrite it to improve it.

[davmac314]

Hi @mobin-2008 , I'd really like you to finish this before opening any other pull requests please. I've already invested review time into it, I don't want that wasted, and this has taken too long for a small change (I know you've had a lot going on, but still, this should be finished before moving on to other things).

The grammar in comments doesn't have to be perfect but you should make sure comments are accurate, clear, and contain appropriate details. You should also switch to comparing each of the *_FOR_BUILD variables as you suggested in your last comment. That's not much to do, there's no reason not to just finish this off. Thanks.

[mobin-2008]

Hi @mobin-2008 , I'd really like you to finish this before opening any other pull requests please. I've already invested review time into it, I don't want that wasted, and this has taken too long for a small change (I know you've had a lot going on, but still, this should be finished before moving on to other things).

The grammar in comments doesn't have to be perfect but you should make sure comments are accurate, clear, and contain appropriate details. You should also switch to comparing each of the *_FOR_BUILD variables as you suggested in your last comment. That's not much to do, there's no reason not to just finish this off. Thanks.

Ok. I'm on it.

[mobin-2008]

I tested it with cports building and it correctly assumes the build is native because of equal environment for both specified compilers.

Also I think configure should always write *FLAGS_FOR_BUILD variables to mconfig even when they are empty to avoid issues further and I will open a PR to fix that after this getting merged.

[davmac314]

Other than the specific comments, I think this also needs documentation to be updated.

[davmac314]

The comment isn't very clear; it doesn't explain the connection between assuming the build is native and unsetting CXX_FOR_BUILD. I already made a very similar comment about this in the previous iteration:

it doesn't say how we "use" $CXX_FOR_BUILD for detecting a cross build, and it doesn't say how/why unsetting the variable alters the result of that detection. For example it doesn't explain the variable is unset just before the check for whether the $CXX_FOR_BUILD is set or not (which is used to detect a cross build). That's so easy to say and makes it so much clearer.

The text "their respective flags" is also vague (who is the "they" corresponding to "their"?), it would be better to be more specific.

[davmac314]

I have a few thoughts about the approach here. I guess you try to derive the full set of flags that will be used for regular compilation (i.e. both from CXXCFLAGS and CXXFLAGS_EXTRA) and compare with the flags for build (CXXFLAGS_FOR_BUILD ) to check whether the compiler flags that will ultimately be used for building application code and host code are the same (a brief comment to explain this would have been good, but that might be moot, as per following).

Mainly, I think this is too complex. We are anyway taking a heuristic approach (if flags differ we don't know for sure if it is or isn't a cross-build) and having overcomplicated heuristics isn't worth it. Some ideas for simplifying it:

First, the point of setting CXXFLAGS_EXTRA is to use the automatically-derived (default) flags for building application code (and add some additional, i.e. EXTRA, flags). Someone using this functionality is unlikely to go to the trouble of then specifying the exact same set of flags in CXXFLAGS_FOR_BUILD. So, it doesn't make a lot of sense to me to even consider CXXFLAGS_EXTRA in this equation, or if you do it should be more along the lines of "if CXXFLAGS_EXTRA is set, assume it might be setting compilation target and so it may be a cross-build".

Second, building for a different target can't be done by changing only the link stage. So I don't think there's much point doing the LDFLAGS comparison.

[davmac314]

So, it doesn't make a lot of sense to me to even consider CXXFLAGS_EXTRA

In a similar vein, it doesn't make much sense to compare a configure-determined CXXFLAGS value with the CXXFLAGS_FOR_BUILD value. If the user didn't set CXXFLAGS and didn't set CXXFLAGS_FOR_BUILD, should they really be considered different?

[mobin-2008]

First, the point of setting CXXFLAGS_EXTRA is to use the automatically-derived (default) flags for building application code (and add some additional, i.e. EXTRA, flags). Someone using this functionality is unlikely to go to the trouble of then specifying the exact same set of flags in CXXFLAGS_FOR_BUILD.

And I'm not assuming that. My case for such logic is this:

./configure CXX="clang++" CXX_FOR_BUILD="clang++" CXXFLAGS_EXTRA="--target=aarch64-linux-gnu"

This logic is there to detect any difference between host compiler and build compiler and CXXFLAGS_EXTRA would make a difference.

So, it doesn't make a lot of sense to me to even consider CXXFLAGS_EXTRA in this equation, or if you do it should be more along the lines of "if CXXFLAGS_EXTRA is set, assume it might be setting compilation target and so it may be a cross-build".

Now I'm thinking, yeah, that make sense to change it to "if CXXFLAGS_EXTRA is set, assume it might be setting compilation target and so it may be a cross-build".

In a similar vein, it doesn't make much sense to compare a configure-determined CXXFLAGS value with the CXXFLAGS_FOR_BUILD value. If the user didn't set CXXFLAGS and didn't set CXXFLAGS_FOR_BUILD, should they really be considered different?

No. They are not different and current logic just assume the build is native in that case because the CXXFLAGS is not utilized by configure at that point and the script never sets any *_FOR_BUILD variable so the check would be [ "" = "" ] && unset CXX_FOR_BUILD.

[mobin-2008]

Second, building for a different target can't be done by changing only the link stage. So I don't think there's much point doing the LDFLAGS comparison.

You're right, checking for compiler flags is enough.

[davmac314]

No. They are not different and current logic just assume the build is native in that case because the CXXFLAGS is not utilized by configure at that point

You are right, I thought this new logic was added after setting CXXFLAGS with default flags, but it is not.

[davmac314]

I tested it with cports building and it correctly assumes the build is native because of equal environment for both specified compilers.

Ok, but what other configurations did you test? Seems to me like:

./configure CXX=gcc CXX_FOR_BUILD=gcc

... is going to be detected as a cross-build.

In general it feels to me like you are now rushing this PR. Please don't; take the time to get it right.

Also I think configure should always write *FLAGS_FOR_BUILD variables to mconfig even when they are empty to avoid issues further and I will open a PR to fix that after this getting merged.

I don't know what you mean. What issues will this avoid?

[mobin-2008]

I tested it with cports building and it correctly assumes the build is native because of equal environment for both specified compilers.

Ok, but what other configurations did you test? Seems to me like:

./configure CXX=gcc CXX_FOR_BUILD=gcc

... is going to be detected as a cross-build.

No.

./configure CXX=gcc CXX_FOR_BUILD=gcc
Checking whether "gcc" is a working C++ compiler...
  ... Yes.
Checking whether "-std=c++11" is accepted by the compiler...
  ... Yes.
Checking whether "-Wall" is accepted by the compiler...
  ... Yes.
Checking whether "-Os" is accepted by the compiler...
  ... Yes.
Checking whether "-fno-plt" is accepted by the compiler...
  ... Yes.
Checking whether "-fno-rtti" is accepted by the compiler...
  ... Yes.
Checking whether "-fno-rtti" breaks exceptions...
  ... Yes. Disabling -fno-rtti
Checking whether "-flto" is accepted as an option for both compiling and linking...
  ... Yes.
Looking for "libcap" via pkg-config...
  ... Found.
Checking whether header "linux/ioprio.h" is available...
  ... Yes.
Checking whether "-fsanitize=address,undefined" is accepted as an option for both compiling and linking...
  ... Yes.
Creating mconfig...
  ... done.
Done!

Because the check

        if [ "$FULL_CXXFLAGS" = "${CXXFLAGS_FOR_BUILD:-}" ]; then
            unset CXX_FOR_BUILD
        fi

in this case will be

        if [ "" = "" ]; then
            unset CXX_FOR_BUILD
        fi

which is true.

In general it feels to me like you are now rushing this PR. Please don't; take the time to get it right.

I'm trying to balance the correctness with time to avoid taking too much time for a simple PR.

Also I think configure should always write *FLAGS_FOR_BUILD variables to mconfig even when they are empty to avoid issues further and I will open a PR to fix that after this getting merged.

I don't know what you mean. What issues will this avoid?

The problem is that if I generate mconfig with:

./configure CXX=clang++ CXX_FOR_BUILD=clang++ CXXFLAGS="-std=c++11 --target=aarch64-linux-gnu"

and then try to build the app, it will use clang++ -std=c++11 --target=aarch64-linux-gnu -o mconfig-gen mconfig-gen.cc for building mconfig-gen.cc which is wrong and will result in build failure. The Makefile uses $CXXFLAGS for compiler flags in this case, causing this issue.

[mobin-2008]

Other than the specific comments, I think this also needs documentation to be updated.

Where? configure help message?

[davmac314]

[...] Seems to me like:

./configure CXX=gcc CXX_FOR_BUILD=gcc

... is going to be detected as a cross-build.

No.

Ok.

In general it feels to me like you are now rushing this PR. Please don't; take the time to get it right.

I'm trying to balance the correctness with time to avoid taking too much time for a simple PR.

In this project correctness should be considered paramount.

Not considering things properly is what leads to taking too much time for the PR. You also ignored one of my earlier review comments (which I pointed out again this review); this is not making things faster, it is making it slower, and is using up my time as well as yours.

Also I think configure should always write *FLAGS_FOR_BUILD variables to mconfig even when they are empty to avoid issues further and I will open a PR to fix that after this getting merged.

The problem is that if I generate mconfig with:

./configure CXX=clang++ CXX_FOR_BUILD=clang++ CXXFLAGS="-std=c++11 --target=aarch64-linux-gnu"

and then try to build the app, it will use clang++ -std=c++11 --target=aarch64-linux-gnu -o mconfig-gen mconfig-gen.cc for building mconfig-gen.cc which is wrong and will result in build failure. The Makefile uses $CXXFLAGS for compiler flags in this case, causing this issue.

I think they should be written to mconfig if they are set (even if set to empty string), not always.

[davmac314]

I'm happy for you to go ahead with changes to address the review. I think you may need to rebase onto main in order to pull the documentation (BUILD) changes so that you can updated them accordingly - please also go ahead and do that.

[davmac314]

Other than the specific comments, I think this also needs documentation to be updated.

Where? configure help message?

BUILD.

If cross-compiling (i.e. specifying the CXX_FOR_BUILD variable), there are some additional
considerations regarding use of the "configure" script.

[mobin-2008]

Ok. I will apply changes tomorrow (It's 2 AM in here) and also make sure to don't rush the process to avoid missing any review points and ensuring correctness.

[davmac314]

Ok. I will apply changes tomorrow (It's 2 AM in here) and also make sure to don't rush the process to avoid missing any review points and ensuring correctness.

Thanks. It doesn't have to be done by tomorrow - get it right rather than do it quickly, please.

[mobin-2008]

I tested it with cports and other cases (./configure CXX=gcc CXX_FOR_BUILD=gcc, CXX="clang++" CXX_FOR_BUILD="clang++" CXXFLAGS_EXTRA="--target=aarch64-linux-gnu" and ./configure CXX=clang++ CXX_FOR_BUILD=clang++ CXXFLAGS="-std=c++11 --target=aarch64-linux-gnu") and it works.

BUILD updated to reflect the how configure reacts to the CXX_FOR_BUILD and other compiler flags for cross-compile detection. Comment improved to properly explain connection between cross-build and $CXX_FOR_BUILD and also used your own comment for giving context in CXXFLAGS_EXTRA check.