Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "arch: Replace ar and nm with gcc-ar and gcc-nm" #4477

Merged
merged 1 commit into from
Sep 7, 2021

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Sep 5, 2021

Summary

This reverts commit b05737d.

Because it broke clang-based builds.

Impact

Testing

This reverts commit b05737d.

Because it broke clang-based builds.
@xiaoxiang781216
Copy link
Contributor

Summary

This reverts commit b05737d.

Because it broke clang-based builds.

@yamt could you point out which config break? https://github.com/apache/incubator-nuttx/pull/4451/checks show that the change pass CI on macOS.

@yamt
Copy link
Contributor Author

yamt commented Sep 6, 2021

Summary

This reverts commit b05737d.
Because it broke clang-based builds.

@yamt could you point out which config break? https://github.com/apache/incubator-nuttx/pull/4451/checks show that the change pass CI on macOS.

clang build of sim/linux at least.
i don't think we are testing it on the ci.

@yamt
Copy link
Contributor Author

yamt commented Sep 6, 2021

i suppose LTO build will be an optional config.
i guess it makes more sense to make these conditional on, say, CONFIG_GCC_LTO.
how do you think?

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Sep 6, 2021

i suppose LTO build will be an optional config.
i guess it makes more sense to make these conditional on, say, CONFIG_GCC_LTO.

Could be, but b05737d doesn't really enable lto, but make enable lto more easier.

how do you think?

For sim clang support, the better fix is SIM_TOOLCHAIN_CLANG/SIM_TOOLCHAIN_GCC and arch/sim/Toolchain.defs.

@yamt
Copy link
Contributor Author

yamt commented Sep 6, 2021

i suppose LTO build will be an optional config.
i guess it makes more sense to make these conditional on, say, CONFIG_GCC_LTO.

Could be, but b05737d doesn't really enable lto, but make enable lto more easier.

how do you think?

For sim clang support, the better fix is SIM_TOOLCHAIN_CLANG/SIM_TOOLCHAIN_GCC and arch/sim/Toolchain.defs.

i'm not sure it's better or not. but i agree it's consistent with the other boards.
are you going to fix this that way?

@yamt
Copy link
Contributor Author

yamt commented Sep 6, 2021

i suppose LTO build will be an optional config.
i guess it makes more sense to make these conditional on, say, CONFIG_GCC_LTO.

Could be, but b05737d doesn't really enable lto, but make enable lto more easier.

how do you think?

For sim clang support, the better fix is SIM_TOOLCHAIN_CLANG/SIM_TOOLCHAIN_GCC and arch/sim/Toolchain.defs.

i'm not sure it's better or not. but i agree it's consistent with the other boards.
are you going to fix this that way?

or are you asking me to do it? either way is ok for me.

@xiaoxiang781216
Copy link
Contributor

We can add an issue. The change isn't huge if you don't have free time, I can do it in this week.

@yamt
Copy link
Contributor Author

yamt commented Sep 6, 2021

after thinking a bit, i guess the problem is not only for sim.
for example, it shouldn't be used in case of CONFIG_ARMV7M_TOOLCHAIN=CLANG, right?
i prefer this to be reverted for now unless we can fix it quickly.
(i don't think i can fix and test non-sim bits so quickly)

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Sep 6, 2021

after thinking a bit, i guess the problem is not only for sim.
for example, it shouldn't be used in case of CONFIG_ARMV7M_TOOLCHAIN=CLANG, right?
i prefer this to be reverted for now unless we can fix it quickly.
(i don't think i can fix and test non-sim bits so quickly)

Fix by #4482

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Sep 6, 2021

@yamt, since #4451 is related to #3836. I would propose to revert them in batch. Also, it's better to make a demo how to enable lto without gcc driver.

@yamt
Copy link
Contributor Author

yamt commented Sep 7, 2021

@yamt, since #4451 is related to #3836. I would propose to revert them in batch. Also, it's better to make a demo how to enable lto without gcc driver.

lto thing has been on my low-priority todo for a while. if anyone can beat me, it's welcome.

otoh, this one is more critical as it's breaking my (private) ci and workflows right now.

if you are going to merge #3836 without waiting for "lto demo", it's fine for me. (IMO it isn't reasonable to require me the demo in the first place.)
otherwise, let's fix this independently from #3836 .

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Sep 7, 2021

@yamt, since #4451 is related to #3836. I would propose to revert them in batch. Also, it's better to make a demo how to enable lto without gcc driver.

lto thing has been on my low-priority todo for a while. if anyone can beat me, it's welcome.

otoh, this one is more critical as it's breaking my (private) ci and workflows right now.

if you are going to merge #3836 without waiting for "lto demo", it's fine for me. (IMO it isn't reasonable to require me the demo in the first place.)

It's fine without lto demo.

otherwise, let's fix this independently from #3836 .

Ok, let's merge this PR first.

@xiaoxiang781216 xiaoxiang781216 merged commit 5ad1cba into apache:master Sep 7, 2021
@Ouss4 Ouss4 added this to To-Add in Release Notes - 10.2 Oct 11, 2021
@jerpelea jerpelea moved this from To-Add to fixes in Release Notes - 10.2 Oct 13, 2021
@Ouss4 Ouss4 moved this from fixes to Not Applicable in Release Notes - 10.2 Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Release Notes - 10.2
Not Applicable
Development

Successfully merging this pull request may close these issues.

None yet

2 participants