Skip to content

Introduce option to use clang++, and option to skip apt packages#971

Merged
rdmark merged 4 commits intodevelopfrom
rdmark-clang-option
Nov 8, 2022
Merged

Introduce option to use clang++, and option to skip apt packages#971
rdmark merged 4 commits intodevelopfrom
rdmark-clang-option

Conversation

@rdmark
Copy link
Copy Markdown
Member

@rdmark rdmark commented Nov 6, 2022

  • Make clang++-11 the default for compiling rascsi
  • Add --with_gcc option to use g++ instead
  • Add option to skip apt-get package installation (this saves a significant amount of time running the script on a Zero when you already have the required packages installed)
  • Add a line to the script menu showing current options
  • Surrounding cleanup

Copy link
Copy Markdown
Contributor

@uweseimet uweseimet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not find apt-install for clang++ in the changes. I don't think it is installed per default, but I'm not sure.

@rdmark
Copy link
Copy Markdown
Member Author

rdmark commented Nov 6, 2022

I did not find apt-install for clang++ in the changes. I don't think it is installed per default, but I'm not sure.

Good catch, I forgot about that. Added now.

@rdmark rdmark requested a review from uweseimet November 6, 2022 17:09
Copy link
Copy Markdown
Contributor

@uweseimet uweseimet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that clang also installs clang++ and that the package name is actually correct? On some platforms the package name referts to llvm, which clang is part of.

@rdmark
Copy link
Copy Markdown
Member Author

rdmark commented Nov 6, 2022

@uweseimet I believe clang is a meta package that pulls in the latest stable C, C++, and Obj-C compilers.

$ sudo apt install clang --no-install-recommends
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
The following additional packages will be installed:
  clang-11 libclang-common-11-dev libclang-cpp11 libclang1-11 libllvm11 libobjc-10-dev libz3-4
Suggested packages:
  clang-11-doc
Recommended packages:
  llvm-11-dev libomp-11-dev
The following NEW packages will be installed:
  clang clang-11 libclang-common-11-dev libclang-cpp11 libclang1-11 libllvm11 libobjc-10-dev libz3-4
0 upgraded, 8 newly installed, 0 to remove and 7 not upgraded.
Need to get 37.3 MB of archives.
After this operation, 190 MB of additional disk space will be used.
Do you want to continue? [Y/n]

@akuker
Copy link
Copy Markdown
Member

akuker commented Nov 6, 2022

I don't think I'm on board with having easyinstall install clang. This is going to take extra space and extra time to install it on everyone's raspberry pi, when very very few people are using it.

@uweseimet - is your intention to switch over completely to clang++? If so, then I'm OK with leaving it in. But then I would expect a PR coming to switch the Makefile over.

@uweseimet
Copy link
Copy Markdown
Contributor

@akuker As I already mentioned in my ticket, I'm totally fine with rejecting it. This is just a low prioriity suggestion, not more than that.
In general, easyinstall should install only what's needed according to the options the user has selected. If it still installs everything that might potentially be needed I suggest to improve this.

@rdmark
Copy link
Copy Markdown
Member Author

rdmark commented Nov 6, 2022

As long as we provide this as "experimental" feature I agree it would make sense to install clang only when explicitly chosen. Save the majority of users the overhead and bloat of having those packags.

@uweseimet
Copy link
Copy Markdown
Contributor

uweseimet commented Nov 6, 2022

I leave the decision to you. I don't have a strong opinion on this. I mainly created this ticket because @akuker mentioned that some users (still) have issues with compiling due to low memory, and I verified that there are no such issues with clang++.

@akuker
Copy link
Copy Markdown
Member

akuker commented Nov 7, 2022

Let's do it! Let's switch to clang :-)

@akuker
Copy link
Copy Markdown
Member

akuker commented Nov 7, 2022

@uweseimet - would you like to update the Makefile? Or should I?

@rdmark
Copy link
Copy Markdown
Member Author

rdmark commented Nov 7, 2022

@akuker I suggest we hold off on this PR until the Makefile changes have been made. Do we want to offer gcc as a command line option for easyinstall, e.g. --use-gcc, for users running very old Raspbian versions that might lack a compatible clang compiler?

@rdmark rdmark requested a review from akuker November 7, 2022 22:03
@rdmark
Copy link
Copy Markdown
Member Author

rdmark commented Nov 7, 2022

So since it seems we will need g++ in the Makefile for CI/CD for the time being (more research and testing required) I propose what we do in this PR, is to pivot to clang-11 as the default options in easyinstall and keep gcc in the Makefile. This gives us the benefit of the performance boost for users upgrading their RaSCSI installations, without breaking UTs and static analysis in the short term.

Thoughts?

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Nov 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@akuker
Copy link
Copy Markdown
Member

akuker commented Nov 8, 2022

Sounds good to me!

@rdmark rdmark merged commit e8b72c4 into develop Nov 8, 2022
@rdmark rdmark deleted the rdmark-clang-option branch November 8, 2022 02:59
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