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 issue related to the installation of Python #236

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

rolandoquesada
Copy link
Contributor

The current setup_vdms.sh script is not installing the new version of Python due to an issue related to when comparing the current version value to the requested version. Also it is needed to update some of the symbolic links to point to the new Python version directory

cwlacewe
cwlacewe previously approved these changes Oct 17, 2024
Copy link
Contributor

@cwlacewe cwlacewe left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks!

@ifadams
Copy link
Contributor

ifadams commented Oct 17, 2024

@cwlacewe @rolandoquesada is there any risk that this might cause problems for other programs relying on a predictable python version set in $PATH?

The script file was updated to only "upgrade" the version of Python
when the current version is lower than the minimum version needed

Also some other script files were updated to call python3 only
@rolandoquesada
Copy link
Contributor Author

rolandoquesada commented Oct 17, 2024

@cwlacewe @rolandoquesada is there any risk that this might cause problems for other programs relying on a predictable python version set in $PATH?
Hi.
Yeah, you're right. I just uploaded the changes to call to the "python3" version instead of "python".
Also I modified the setup_vdms.sh script to install a newer version of Python when the current python3 version is older than the minimum required.
Finally, I modified the INSTALL.md file to indicate that VMDS requires at least the Python 3,12+ version.

Please let me know if you have any other suggestions. Thank you.

@ifadams
Copy link
Contributor

ifadams commented Oct 17, 2024

Please let me know if you have any other suggestions. Thank you

@rolandoquesada I think my bigger concern is that updating Python has the potential to break existing dependencies if we're not careful, unless this is all being done within the docker container or a virtual environment.

Copy link

Target CPP Coverage: 63.8504%
Source CPP Coverage: 63.8406%

Target Python Coverage: 98.02%
Source Python Coverage: 98.02%

docker/base/Dockerfile Outdated Show resolved Hide resolved
Copy link

Target CPP Coverage: 63.8504%
Source CPP Coverage: 63.8504%

Target Python Coverage: 98.02%
Source Python Coverage: 98.02%

@rolandoquesada
Copy link
Contributor Author

Please let me know if you have any other suggestions. Thank you

@rolandoquesada I think my bigger concern is that updating Python has the potential to break existing dependencies if we're not careful, unless this is all being done within the docker container or a virtual environment.

Hi Ian, thanks for the feedback, @cwlacewe as I was fixing an issue related to the condition for installing

Please let me know if you have any other suggestions. Thank you

@rolandoquesada I think my bigger concern is that updating Python has the potential to break existing dependencies if we're not careful, unless this is all being done within the docker container or a virtual environment.

Hi Ian, thanks for the feedback. If that's the case, I think I could remove the lines for updating the path to Python in /usr/bin. However, now I am wondering if we should keep the conditional that checks if we need to install a new version of Python (that condition was before my new changes). @cwlacewe I am wondering what you think about it?

The code will be as follows:
python_installation

Copy link

Target CPP Coverage: 63.8504%
Source CPP Coverage: 63.8504%

Target Python Coverage: 98.02%
Source Python Coverage: 98.02%

@cwlacewe
Copy link
Contributor

Please let me know if you have any other suggestions. Thank you

@rolandoquesada I think my bigger concern is that updating Python has the potential to break existing dependencies if we're not careful, unless this is all being done within the docker container or a virtual environment.

Hi Ian, thanks for the feedback, @cwlacewe as I was fixing an issue related to the condition for installing

Please let me know if you have any other suggestions. Thank you

@rolandoquesada I think my bigger concern is that updating Python has the potential to break existing dependencies if we're not careful, unless this is all being done within the docker container or a virtual environment.

Hi Ian, thanks for the feedback. If that's the case, I think I could remove the lines for updating the path to Python in /usr/bin. However, now I am wondering if we should keep the conditional that checks if we need to install a new version of Python (that condition was before my new changes). @cwlacewe I am wondering what you think about it?

The code will be as follows: python_installation

@rolandoquesada I think we should check if 3.12 exists, if so, make it the default python3 version; otherwise install and do the same.
@ifadams do you have concerns with this regarding making 3.12 the python3 default version? Do you think we should create a virtual env on the user's system instead of changing the default version?

@ifadams
Copy link
Contributor

ifadams commented Oct 18, 2024

@ifadams do you have concerns with this regarding making 3.12 the python3 default version? Do you think we should create a virtual env on the user's system instead of changing the default version?

@cwlacewe I dont have a very concrete concern so much as my own experience that in bare-metal installs updating python can be challenging in unpredictable ways. Sometimes "just works" other times explodes. Seems to depend on vagaries of the host environment.

My thought might be that we focus on the automation for container build, and either need a prompt or have just a note that "You need to install python 3.12 for host installation" in the docs. Ill totally admit virtual environments are a mystery to me.

Thoughts?

@cwlacewe
Copy link
Contributor

@cwlacewe I dont have a very concrete concern so much as my own experience that in bare-metal installs updating python can be challenging in unpredictable ways. Sometimes "just works" other times explodes. Seems to depend on vagaries of the host environment.

My thought might be that we focus on the automation for container build, and either need a prompt or have just a note that "You need to install python 3.12 for host installation" in the docs. Ill totally admit virtual environments are a mystery to me.

Thoughts?

That would work. We can remove the python installation from script but check whether 3.12+ is installed. If it isn't installed, we can exit script with note advising to install it. If it is installed, we can use the newest version >= 3.12. How does that sound @ifadams @rolandoquesada?

@ifadams
Copy link
Contributor

ifadams commented Oct 18, 2024

That would work. We can remove the python installation from script but check whether 3.12+ is installed. If it isn't installed, we can exit script with note advising to install it. If it is installed, we can use the newest version >= 3.12. How does that sound @ifadams @rolandoquesada?

That works for me. I may be exercising an overabundance of caution, FWIW.

@rolandoquesada
Copy link
Contributor Author

That would work. We can remove the python installation from script but check whether 3.12+ is installed. If it isn't installed, we can exit script with note advising to install it. If it is installed, we can use the newest version >= 3.12. How does that sound @ifadams @rolandoquesada?

That works for me. I may be exercising an overabundance of caution, FWIW.

That works for me as well, I am going to do it in that way. Thank you @ifadams @cwlacewe

Copy link

github-actions bot commented Nov 6, 2024

Target CPP Coverage: 64.1637%
Source CPP Coverage: 64.1637%

Target Python Coverage: 98.02%
Source Python Coverage: 98.02%

Copy link

github-actions bot commented Nov 6, 2024

Target CPP Coverage: 64.1637%
Source CPP Coverage: 64.1637%

Target Python Coverage: 98.02%
Source Python Coverage: 98.02%

Copy link

github-actions bot commented Nov 6, 2024

Target CPP Coverage: 64.1637%
Source CPP Coverage: 64.1539%

Target Python Coverage: 98.02%
Source Python Coverage: 98.02%

@cwlacewe cwlacewe added this to the v2.11.0 Tasks milestone Nov 6, 2024
Copy link

github-actions bot commented Nov 6, 2024

Target CPP Coverage: 64.1637%
Source CPP Coverage: 64.1637%

Target Python Coverage: 98.02%
Source Python Coverage: 98.02%

Copy link

github-actions bot commented Nov 7, 2024

Target CPP Coverage: 64.1637%
Source CPP Coverage: 64.1637%

Target Python Coverage: 98.02%
Source Python Coverage: 98.02%

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.

4 participants