Skip to content

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

Closed
Junchao-Mellanox wants to merge 7 commits intosonic-net:masterfrom
Junchao-Mellanox:upgrade-cpp17
Closed

[sonic-sairedis] Upgrade to c++17#1069
Junchao-Mellanox wants to merge 7 commits intosonic-net:masterfrom
Junchao-Mellanox:upgrade-cpp17

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 ?

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

- 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