configure: fix broken 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.