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

[sonic-sairedis] Upgrade to c++17 #1069

Closed

Conversation

Junchao-Mellanox
Copy link
Contributor

What I did?
Upgrade sonic-sairedis cpp compiler option to c++17.

What I test?
Build test

@keboliu keboliu requested a review from kcudnik June 27, 2022 01:28
kcudnik
kcudnik previously approved these changes Jun 27, 2022
@kcudnik
Copy link
Collaborator

kcudnik commented Jun 27, 2022

could you please check errors ?

@@ -157,6 +157,7 @@ CXXFLAGS_COMMON+=" -Wwrite-strings"
CXXFLAGS_COMMON+=" -Wno-switch-default"
CXXFLAGS_COMMON+=" -Wconversion"
CXXFLAGS_COMMON+=" -Wno-psabi"
CXXFLAGS_COMMON+=" -Wno-register"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this? I search codebase and did not find register storage class specifier.
ref: https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Dialect-Options.html#index-Wno-register

@@ -37,7 +37,7 @@ resources:
parameters:
- name: debian_version
type: string
default: buster
default: bullseye
Copy link
Collaborator

Choose a reason for hiding this comment

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

on .azure-pipelines/build-swss-template.yml tou set explicit target to buster, but here to bullseye, will that be a conflict?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have to do this because there is no other pipeline that are using bullseye. So, if I do not set explicit target to buster in .azure-pipelines/build-swss-template.yml, it cannot download dependencies like "libnl*".

@Junchao-Mellanox
Copy link
Contributor Author

Hi @kcudnik @qiluo-msft , the current checker failed because of:

dpkg: dependency problems prevent configuration of libsairedis:
 libsairedis depends on libgcc-s1 (>= 3.0); however:
  Package libgcc-s1 is not installed.
 libsairedis depends on libstdc++6 (>= 9); however:
  Version of libstdc++6:amd64 on system is 8.3.0-6.

The root cause is that, docker-sonic-vs is based on buster, it has no libgcc-s1, it has gcc 8.3.0. I also found that all other sonic submodule pipelines are using buster now, so, if I only upgrade pipeline to bullseye for sonic-sairedis, there are a lot of dependency issues need to be resolved. I feel like the work effort is much larger than I thought. There are two options on my mind:

  1. I have to drop this PR and continue using c++14. In that case, I will need to change some code in PR [FlexCounter] Refactor FlexCounter class  #1073 to make compile happy.
  2. Upgrade all relevant azure pipelines to bullseye, I cannot estimate the work effort as I am not so familiar with it.

So, I would prefer option 1 for now. Any comment?

@kcudnik
Copy link
Collaborator

kcudnik commented Jul 3, 2022

i think if you continue on cpp14 it will be much safer than changing all pipelines and potentially causing more trouble, and it's too much effort just to use constexpr if from cpp17

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