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

Fix py3 version changed even version control enabled issue #6422

Merged
merged 3 commits into from
Jan 13, 2021

Conversation

xumia
Copy link
Collaborator

@xumia xumia commented Jan 12, 2021

- Why I did it
The pip3 is installed the /usr/local/bin, the hook command pip3 does not work correctly. Change to use /usr/local/sbin.

- How I did it

- How to verify it
Enable the version control and set the version of the pip package importlib-metadata to 3.3.0, run the following command.
pip3 install importlib-metadata
Before fixing the issue, the version 3.4.0 is installed.
After fixing the issue, the version 3.3.0 installed as expected.

- Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@@ -2,6 +2,7 @@

HOOK_PATH=/usr/local/share/buildinfo/hooks
TARGET_PATH=/usr/sbin
[ -d /usr/local/sbin ] && TARGET_PATH=/usr/local/sbin
Copy link
Collaborator

@qiluo-msft qiluo-msft Jan 13, 2021

Choose a reason for hiding this comment

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

/usr/local/sbin [](start = 5, length = 15)

To simplify the logic, we can assume /usr/local/sbin exists and fail if not. #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Simply change to /usr/local/sbin, we assume the path exists, if not will be failed when creating the symbol link.


In reply to: 556191523 [](ancestors = 556191523)

@qiluo-msft
Copy link
Collaborator

qiluo-msft commented Jan 13, 2021

if [[ ",$SONIC_VERSION_CONTROL_COMPONENTS," == *,all,* ]] || [[ ",$SONIC_VERSION_CONTROL_COMPONENTS," == *,$1,* ]]; then

This looks weird. Could you check the best practice for string comparision? #Closed


Refers to: src/sonic-build-hooks/scripts/buildinfo_base.sh:34 in 27ed107. [](commit_id = 27ed107, deletion_comment = False)

@@ -24,7 +24,7 @@ log_err()

get_command()
Copy link
Collaborator

@qiluo-msft qiluo-msft Jan 13, 2021

Choose a reason for hiding this comment

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

get_command [](start = 0, length = 11)

Could you add function scope comment to explain the intention? #Closed

@xumia
Copy link
Collaborator Author

xumia commented Jan 13, 2021

if [[ ",$SONIC_VERSION_CONTROL_COMPONENTS," == *,all,* ]] || [[ ",$SONIC_VERSION_CONTROL_COMPONENTS," == *,$1,* ]]; then

How about change to grep as below? Or any advice?
if echo ",$SONIC_VERSION_CONTROL_COMPONENTS," | grep -q ",(all|$1),"


In reply to: 759130913 [](ancestors = 759130913)


Refers to: src/sonic-build-hooks/scripts/buildinfo_base.sh:34 in 27ed107. [](commit_id = 27ed107, deletion_comment = False)

@qiluo-msft
Copy link
Collaborator

if [[ ",$SONIC_VERSION_CONTROL_COMPONENTS," == *,all,* ]] || [[ ",$SONIC_VERSION_CONTROL_COMPONENTS," == *,$1,* ]]; then

I mean , is weird.


In reply to: 759140107 [](ancestors = 759140107,759130913)


Refers to: src/sonic-build-hooks/scripts/buildinfo_base.sh:34 in 27ed107. [](commit_id = 27ed107, deletion_comment = False)

@@ -22,15 +22,19 @@ log_err()
echo "$1" 1>&2
}

# Get the real command not hooked by sonic-build-hook package
Copy link
Collaborator

@qiluo-msft qiluo-msft Jan 13, 2021

Choose a reason for hiding this comment

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

And how? I guess you remove some paths (listing them) in $PATH and check the absolute path of the same command again? #Closed

Copy link
Collaborator Author

@xumia xumia Jan 13, 2021

Choose a reason for hiding this comment

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

We add the command in /usr/local/sbin, and exclude the /usr/local/sbin in env PATH when find the real command.


In reply to: 556218199 [](ancestors = 556218199)

@xumia
Copy link
Collaborator Author

xumia commented Jan 13, 2021

retest vs please

@xumia xumia merged commit 1dcab4d into sonic-net:master Jan 13, 2021
lguohan pushed a commit that referenced this pull request Jan 15, 2021
* Fix py3 version changed even version control enabled issue

* Add some comments and simplify the script

* Add the comment to explain how to get the not hooked command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants