technyon / Arduino-CMake-Toolchain

CMake toolchain for all Arduino compatible boards
MIT License
20 stars 7 forks source link

Use ripgrep for calculating firmware size #2

Closed soonick closed 5 months ago

soonick commented 5 months ago

Hello @technyon , this change might be a little controversial, but I wanted to check if you would be interested.

My comment explains what and why.

If this interests you, I can modify the readme so the note is in a better section.

Regards,

technyon commented 5 months ago

I'm on the fence with this PR. I see it's a good improvement, but on the other hand it's another dependency that could potentially could be problematic. Should we maybe make this optional, so that if ripgrep is detected it uses it as an alternative method to determine firmware size?

KSDaemon commented 5 months ago

my 50cents: I agree with @technyon. ripgrep is not so popular and probably is not available out of the box on developers machines. So it would be great to auto-detect it and use if it exists or show gentle notice about it in other case. But making it a hard dep — doesn't look great.

soonick commented 5 months ago

Sounds fair. I should be able to detect ripgrep and use it if available. I'll update the PR in the next days

soonick commented 5 months ago

@technyon , I've made it so ripgrep is only used if available. Please take a look and let me know what you think.

technyon commented 5 months ago

Looks good, I've checked building for an ESP32 without ripgrep, no problems at all.

soonick commented 5 months ago

@technyon , thanks for merging my PR.

This is of course up to you, but I would recommend changing the setting to the project to always amend and rebase, otherwise, the history of the project will become polluted by merge commits.

Here is how it looks after my commit was merged. If it had been rebased, the merge commit wouldn't exist:

Screenshot from 2024-05-24 15-08-19

technyon commented 5 months ago

I'll have a look into it, never really thought about it actually.