DPDK MANA CentOS 7 changes #3373
Conversation
Signed-off-by: Pavel Boldin <[email protected]>
| devtoolset = has_devtoolset[0] | ||
| node.os.install_packages([devtoolset]) | ||
| node.tools[Mv].move("/bin/gcc", "/bin/gcc_back", overwrite=True, sudo=True) | ||
| result.assert_exit_code() |
There was a problem hiding this comment.
add message, so once the assertion failed, the error message give more details.
There was a problem hiding this comment.
Please take care all assert_exit_code, add error message.
| @@ -143,7 +146,7 @@ def install(self) -> str: | |||
| runbook: SourceInstallerSchema = self.runbook | |||
| assert runbook.location, "the repo must be defined." | |||
|
|
|||
| self._install_build_tools(node) | |||
| self._install_build_tools(node, runbook.build_deps) | |||
There was a problem hiding this comment.
Can the build_deps be added into this class, instead of configurable? It makes repro harder from others.
There was a problem hiding this comment.
Can you please elaborate?
Currently it is added as a configurable because different kernel versions might need different build dependencies.
There was a problem hiding this comment.
You can add checks for kernel versions, if it's some version, add the deps. If the deps are maintained out of code, it's hard to others to make the build tools works without the right configuration.
There was a problem hiding this comment.
Yeah, but we fetch the kernel version after installation of the build tools (otherwise make kernelrelease might fail). Should I add a second call to install_build_tools after kernel version is known?
There was a problem hiding this comment.
Yes, you can call installation again, after know kernel version. Please add comments to explain the dependencies.
|
@paboldin Thank you for contribution! Please agree the CLA, so I can trigger checks. |
|
@microsoft-github-policy-service agree company="CloudLinux" |
Signed-off-by: Pavel Boldin <[email protected]>
Signed-off-by: Pavel Boldin <[email protected]>
Signed-off-by: Pavel Boldin <[email protected]>
Signed-off-by: Pavel Boldin <[email protected]>
Signed-off-by: Pavel Boldin <[email protected]>
Signed-off-by: Pavel Boldin <[email protected]>
paboldin
force-pushed
the
pboldin/dpdk-mana-centos-7
branch
from
a1137e7
to
648f442
Compare
|
@squirrelsc done, and I believe addressed most of your comments. Removed the top commit with "return True" on SRIOV check. Thanks for the review! EDIT: comment -> commit |
|
@paboldin , I just found it's not merged into main. Can you direct the changes to main branch? |
I'm afraid not, underlying branch is necessary. |
|
@mcgov what's the plan to send PR for dpdk-mana-merge? If the PR goes to dpdk-mana-merge, it needs review again, when it's merged to main. If we have to do, please make sure each commit is meaningful and well managed. |
| result = node.execute( | ||
| f"ln -s /opt/rh/{devtoolset}/root/usr/bin/gcc /bin/gcc", sudo=True | ||
| ) | ||
| result.assert_exit_code(f"can not link {devtoolset} to gcc") |
There was a problem hiding this comment.
Use named arguments, so it won't be broken, if the interface changed in future.
|
@paboldin any plan to update this PR? |
|
@LiliDeng I'm on it. Any more comments for review? |
Signed-off-by: Pavel Boldin <[email protected]>
Signed-off-by: Pavel Boldin <[email protected]>
paboldin
force-pushed
the
pboldin/dpdk-mana-centos-7
branch
from
648f442
to
af1fef1
Compare
|
@mcgov can you continue the work on microsoft:mcgov/dpdk-mana-merge? There may be many conflicts with main branch. |
|
@paboldin Thank you for contribution! Would you split tool, installer changes in separated PRs to main branch? The dpdk is changed a lot, so mcgov may need more time to merge the private branch firstly. |
This patchset is built on top of
mcgov/dpdk-mana-mergebranch and provides support to build DPDK and RDMA-CORE on CentOS 7.0 Azure images.