Skip to content

Conversation

@Ouss4
Copy link
Member

@Ouss4 Ouss4 commented May 4, 2022

Summary

This reverts commit 05ff19d because now we are having a bunch of make: arm-nuttx-eabi-gcc: Command not found for some configs.

I've seen it first in #6197 but every PR has the same errors/messages.

Impact

Remove build error

Testing

CI will do the testing.

@Ouss4 Ouss4 requested a review from xiaoxiang781216 May 4, 2022 16:04
@Ouss4
Copy link
Member Author

Ouss4 commented May 4, 2022

@xiaoxiang781216 the comment says:

Don't skip configure and distclean to improve the test coverage

I'm not sure how does this improve test coverage. Can you please explain it? Maybe we can implement it differently and not just revert your commit.

@xiaoxiang781216
Copy link
Contributor

@xiaoxiang781216 the comment says:

Don't skip configure and distclean to improve the test coverage

I'm not sure how does this improve test coverage. Can you please explain it? Maybe we can implement it differently and not just revert your commit.

Check defconfig in the normalize form which is bypassed before my patch.

@xiaoxiang781216
Copy link
Contributor

From the ci log, it look like that make try to invoke arm-nuttx-eabi-gcc in preconfig and distclean phase, but it shouldn't.

@Ouss4
Copy link
Member Author

Ouss4 commented May 4, 2022

@xiaoxiang781216 the comment says:

Don't skip configure and distclean to improve the test coverage

I'm not sure how does this improve test coverage. Can you please explain it? Maybe we can implement it differently and not just revert your commit.

Check defconfig in the normalize form which is bypassed before my patch.

I see. But it looks like we can't get rid of those errors. The moment we call make, through sethost.sh when configuring, we get those. I tracked it to this: https://github.com/apache/incubator-nuttx/blob/fdef3a7b9220e53d898742251a6f5a721beb557b/arch/arm/src/armv7-m/Toolchain.defs#L259-L274

@xiaoxiang781216
Copy link
Contributor

@xiaoxiang781216 the comment says:

Don't skip configure and distclean to improve the test coverage

I'm not sure how does this improve test coverage. Can you please explain it? Maybe we can implement it differently and not just revert your commit.

Check defconfig in the normalize form which is bypassed before my patch.

I see. But it looks like we can't get rid of those errors. The moment we call make, through sethost.sh when configuring, we get those. I tracked it to this:

https://github.com/apache/incubator-nuttx/blob/fdef3a7b9220e53d898742251a6f5a721beb557b/arch/arm/src/armv7-m/Toolchain.defs#L259-L274

@anchao could we delay the evaluation to the real used place?

@xiaoxiang781216 xiaoxiang781216 requested a review from anchao May 5, 2022 06:44
@anchao
Copy link
Contributor

anchao commented May 5, 2022

@anchao could we delay the evaluation to the real used place?

@xiaoxiang781216 @Ouss4

I added clang's check in case of trying to print the libgcc path, this will get rid of this warning, please review this PR:
#6213

@Ouss4
Copy link
Member Author

Ouss4 commented May 5, 2022

So the issue was the simply expanded variable at line 263: COMPILER_RT_LIB :=

@anchao why do we need that only for clang?

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented May 5, 2022

So the issue was the simply expanded variable at line 263: COMPILER_RT_LIB :=

@anchao why do we need that only for clang?

since only clang has this bug: ARM-software/LLVM-embedded-toolchain-for-Arm#109

@Ouss4
Copy link
Member Author

Ouss4 commented May 5, 2022

So the issue was the simply expanded variable at line 263: COMPILER_RT_LIB :=
@anchao why do we need that only for clang?

since only clang has this bug: ARM-software/LLVM-embedded-toolchain-for-Arm#109

Thanks @xiaoxiang781216 @anchao. We don't need this one anymore, I'll close it.

@Ouss4 Ouss4 closed this May 5, 2022
@Ouss4 Ouss4 deleted the testbuild branch May 5, 2022 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants