-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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-bgpcfgd] Call Python 3 version of sonic-cfggen for testing #5847
Conversation
But currently sonic-config-engines supports only python2. |
Python 3 sonic-config-engine should now be installed everywhere via these PRs: |
Yes. Youre right. That works for me. |
@@ -13,7 +13,7 @@ def run_test(name, template_path, json_path, match_path): | |||
template_path = os.path.join(TEMPLATE_PATH, template_path) | |||
json_path = os.path.join(DATA_PATH, json_path) | |||
cfggen = os.path.abspath("../sonic-config-engine/sonic-cfggen") | |||
command = ['/usr/bin/python2.7', cfggen, "-T", TEMPLATE_PATH, "-t", template_path, "-y", json_path] | |||
command = ['/usr/bin/python3', cfggen, "-T", TEMPLATE_PATH, "-t", template_path, "-y", json_path] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion is to completely remove '/usr/bin/python3' from the command array.
Just cfggen would be enough
Added. That would be enough if you remove
"env={"PYTHONPATH": "."}" from below too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that, but until we remove all Python 2, I want to explicitly call the Python 3 version, so that it doesn't accidentally try to run the Python 2 version.
I think we can clean it up later, once we have removed Python 2 completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pavel-shirshov: When attempting to call sonic-cfggen locally via a relative path, there were missing dependencies. Thus, I made further changes to add a test dependency on the sonic-config-engine package in order to ensure the package is installed in the build environment, and then let the location be resolved via the system path.
However, there are now some test failures. Can you please investigate them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tahmed-dev: I'm curious if these test failures may be related to the new sorting differences since you removed the dependency of sonic-cfggen on natsort. Perhaps you can comment on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. @pavel-shirshov has migrated the package to Python 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tahmed-dev: I have incorporated your changes into my branch. A number of the tests are now passing, but some are still failing. It appears as though you may have used an out-of-date branch. I will try to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All check builds are passing now. Thanks for your help, @tahmed-dev!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great news! Thanks @jleveque!
retest broadcom please |
Can we convert sonic-cfggen into python3? |
…nic-net#5847) sonic-bgpcfgd build fails in the absence of Python 2, as it attempts to explicitly call sonic-cfggen using `/usr/bin/python2.7`. Also, it attempts to call sonic-cfggen using a local, relative path. Since the sonic-config-engine package is not installed, neither are its dependencies. Now, we configure the Python 3 sonic-config-engine as a dependency of sonic-bgpcfgd, which ensures the Python 3 sonic-config-engine package and its dependencies are installed before sonic-bgpcfgd is built/tested.
- Why I did it
sonic-bgpcfgd build fails in the absence of Python 2, as it attempts to explicitly call sonic-cfggen using
/usr/bin/python2.7
. Also, it attempts to call sonic-cfggen using a local, relative path. Since the sonic-config-engine package is not installed, neither are its dependencies.Resolves #5847
- How I did it
Configure the Python 3 sonic-config-engine as a dependency of sonic-bgpcfgd, which ensures the Python 3 sonic-config-engine package and its dependencies are installed before sonic-bgpcfgd is built/tested.
- How to verify it
Build sonic-bgpcfgd.
- Which release branch to backport (provide reason below if selected)
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)