-
Notifications
You must be signed in to change notification settings - Fork 411
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
Adds /home/USER/.local/bin/ to PATH in /etc/sudoers.d/vscode, #887
Adds /home/USER/.local/bin/ to PATH in /etc/sudoers.d/vscode, #887
Conversation
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 minor comment, besides that looks good to me!
A friendly ping @prabhakk-mw, it would be wonderful to merge this PR in, can you help us address one minor comment? Thanks! |
4ccebdd
to
4a25361
Compare
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.
Let one comment, thanks!
@microsoft-github-policy-service agree company="MathWorks" |
@prabhakk-mw Hi 👋 |
Thank you for circling back, I'll make time to push your suggested changes this week. |
@samruddhikhandale |
d18cd85
to
c837f2e
Compare
@samruddhikhandale |
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.
Left some minor comments, thanks!
Co-authored-by: Samruddhi Khandale <[email protected]>
Co-authored-by: Prabhakar Kumar <[email protected]>
d3d4513
to
b9a712f
Compare
The tests are failing even on the Also, it might be worth checking if updating action-runner to |
Are we still waiting for the fixes to the main branch? Thanks |
@prabhakk-mw Thanks for the nudge! Can you update https://github.com/devcontainers/features/blob/main/.github/workflows/test-all.yaml#L63 to |
Yep, however as a standard practice we'd like to make sure the tests are green before we merge in any new PRs. |
@gauravsaini04 Can you prioritize fixing the tests? thanks! |
Hi @samruddhikhandale @gauravsaini04 The reason the CENTOS7 scenario is failing seems to be because of incorrectly configured PATHS for the openssl installation. Reverting to using the default installation paths for it based on their doc seems to have fixed it. A potential fix for the failing diff --git a/src/python/install.sh b/src/python/install.sh
index 1aad3f6..f3eea3d 100755
--- a/src/python/install.sh
+++ b/src/python/install.sh
@@ -390,7 +390,7 @@ add_symlink() {
}
install_openssl3() {
- local _prefix=$1
mkdir /tmp/openssl3
(
cd /tmp/openssl3
@@ -403,7 +403,8 @@ install_openssl3() {
curl -sSL -o "/tmp/openssl3/${tgz_filename}" "${tgz_url}"
tar xzf ${tgz_filename}
cd openssl-${openssl3_version}
- ./config --prefix=${_prefix} --openssldir=${_prefix} --libdir=lib
+ ./config --libdir=lib
make -j $(nproc)
make install_dev
)
@@ -446,11 +447,12 @@ install_from_source() {
# Some platforms/os versions need modern versions of openssl installed
# via common package repositories, for now rhel-7 family, use case statement to
# make it easy to expand
+ INSTALL_PATH="/usr"
case ${VERSION_CODENAME} in
centos7|rhel7)
check_packages perl-IPC-Cmd
- install_openssl3 ${INSTALL_PATH}
- ADDL_CONFIG_ARGS="--with-openssl=${INSTALL_PATH} --with-openssl-rpath=${INSTALL_PATH}/lib"
+ install_openssl3
+ ADDL_CONFIG_ARGS="--with-openssl=${INSTALL_PATH}/local --with-openssl-rpath=${INSTALL_PATH}/local/lib"
;;
esac
With these changes I was able to get the test passing: Result:
I've also verified that all the python tests pass:
|
@samruddhikhandale considering the review speed of the repository I strongly disagree with this workflow. This PR is the only solution proposed to a wider issue which is preventing anyone to use jupyterlab in devcontainers if not using the universal one. It's clearly not changing anything to the tests and by blocking it you prevent everyone using other OS than centOS-7 to use JupyterLab in a devcontainer. Why not opening a dedicating follow-up issue/PR and move on with this one ? I guess like every one of us you have limited resources to dedicate to this project, I created a specific PR applying the solution proposed by @prabhakk-mw: #985 |
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.
⚡ Thank you so much!
fixes #870