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

Make Linux (de)activate scripts standalone like on Windows #91

Merged
merged 12 commits into from
Aug 3, 2022

Conversation

leofang
Copy link
Member

@leofang leofang commented Jul 22, 2022

Supersede #68. Close #68.

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@leofang
Copy link
Member Author

leofang commented Jul 22, 2022

@conda-forge-admin, please rerender

recipe/linux/install_nvcc.sh Outdated Show resolved Hide resolved
conda-forge-webservices[bot] and others added 2 commits July 22, 2022 21:27
@jakirkham
Copy link
Member

Appreciate the effort here Leo, but am not sure this is going to work.

We rely on having some of these environment variables getting replaced while leaving others to be determined at runtime. If we go this route, it may require some patching of the scripts, using sed, and/or injecting the full paths into the scripts for prefix replacement.

If we find something that works here, would be good to do a fair bit of testing to make sure everything is still behaving as expected.

@leofang
Copy link
Member Author

leofang commented Jul 22, 2022

Hmmm I thought I did that, but maybe there're a few other variables that I missed. Please check the Windows scripts. The structure is identical now, so if we can do the variable injection on Windows, we should be able to do that on Linux too.

That said, I cannot continue today for some urgent tasks. Let me mark this as draft.

@leofang leofang marked this pull request as draft July 22, 2022 22:12
@jakirkham
Copy link
Member

Gotcha. Yeah can take a look later

No worries. Thanks for the effort here. Agree this would be more readable/maintainable with this change

@leofang leofang changed the title Make Linux (de)activate scripts standalone like Windows Make Linux (de)activate scripts standalone like on Windows Jul 25, 2022
@leofang leofang marked this pull request as ready for review July 25, 2022 17:38
@leofang
Copy link
Member Author

leofang commented Jul 25, 2022

We rely on having some of these environment variables getting replaced while leaving others to be determined at runtime. If we go this route, it may require some patching of the scripts, using sed, and/or injecting the full paths into the scripts for prefix replacement.

Hi @jakirkham it appears to me that we only need to worry about PKG_VERSION (which was already correctly handled). Am I missing something?

@leofang
Copy link
Member Author

leofang commented Jul 25, 2022

@conda-forge-admin, please restart ci

@jakirkham
Copy link
Member

@conda-forge-admin, please re-render

@github-actions
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/nvcc-feedstock/actions/runs/2737010599.

conda-forge.yml Outdated Show resolved Hide resolved
@leofang
Copy link
Member Author

leofang commented Jul 26, 2022

@conda-forge-admin, please re-render

conda-forge.yml Outdated Show resolved Hide resolved
@jakirkham
Copy link
Member

@galipremsagar @vyasr, could you please help test this when you have a chance?

Prebuilt Conda binaries can be found on CI (relevant docs). More context can also be found in issue ( conda-forge/conda-forge.github.io#1424 ).

@galipremsagar
Copy link
Contributor

@galipremsagar @vyasr, could you please help test this when you have a chance?

Prebuilt Conda binaries can be found on CI (relevant docs). More context can also be found in issue ( conda-forge/conda-forge.github.io#1424 ).

Verified locally with the artifacts and works on my machine 👍

recipe/linux/activate.sh Outdated Show resolved Hide resolved
@jaimergp
Copy link
Member

@leofang Do you think it's worth it to use set -euo pipefail in the (de)activation scripts? I am not sure if you considered some variables might be undefined (like CONDA_BUILD_SYSROOT) under certain circumstances.

Co-authored-by: Jaime Rodríguez-Guerra <jaimergp@users.noreply.github.com>
@leofang
Copy link
Member Author

leofang commented Jul 27, 2022

@leofang Do you think it's worth it to use set -euo pipefail in the (de)activation scripts? I am not sure if you considered some variables might be undefined (like CONDA_BUILD_SYSROOT) under certain circumstances.

I'd like to leave this question to @jakirkham @galipremsagar. In #87 and other PRs we tried to make the (de)activation script work outside of conda-build. My only concern is if the (de)activation script fails conda may be left in an invalid state. I seem to remember seeing a similar issue in the past but can't recall where.

@leofang
Copy link
Member Author

leofang commented Jul 27, 2022

Another note is we currently have both CONDA_BUILD_SYSROOT and CONDA_BUILD used in the script. I aimed to do a simple migration in this PR so I didn't bother to touch them. Let me know if we need to unify the usage in this PR.

recipe/linux/activate.sh Outdated Show resolved Hide resolved
Co-authored-by: jakirkham <jakirkham@gmail.com>
@vyasr
Copy link

vyasr commented Aug 1, 2022

@jakirkham @leofang @galipremsagar I was also able to verify that this worked for me locally.

@jakirkham jakirkham requested a review from jaimergp August 1, 2022 19:11
@jakirkham
Copy link
Member

@jaimergp, could you please take another look when you have a chance? 🙂

@leofang
Copy link
Member Author

leofang commented Aug 1, 2022

@conda-forge-admin, please re-render

@leofang
Copy link
Member Author

leofang commented Aug 3, 2022

Let's get this merged. Thanks everyone for reviewing & testing!

@leofang leofang merged commit 65f2fb0 into conda-forge:main Aug 3, 2022
@leofang leofang deleted the standalone branch August 3, 2022 14:47
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.

6 participants