-
Notifications
You must be signed in to change notification settings - Fork 386
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
Add a feature for Haskell (GHC) #470
base: main
Are you sure you want to change the base?
Conversation
Hi @brendandburns, thanks so much for the PR! We try to keep a small subset of languages in this repo. We then absolutely welcome further community-published contributions to our Feature index. Would you be interested in publishing this Feature in your own repo (i.e. at https://github.com/brendandburns/ghc-feature) and then opening a PR to update our index? We have a sample repo that walks through the process. Let us know if you have any questions, thank you! |
@bamurtaugh I'm happy to put it in a separate repo and indeed, I've already published some features here, so I'm familiar with the process. https://github.com/dev-wasm/dev-wasm-feature However in this case it seems odd to have an independent repo for a (relatively) mainstream language, since it will lead to a variety of different people implementing the same things. How does the team decide which languages to support? I'm happy to sign up to be a maintainer of this. However, if it truly doesn't fit in this repo, I will do it as a separate feature. |
Thanks for opening this dialogue, this is a great question. For broader context (especially if other folks may discover this PR): We previously hosted all dev container templates and features in one large mono-repo: https://github.com/microsoft/vscode-dev-containers/tree/main/containers. This became difficult to manage as our maintainer group doesn't have expertise in each language, and it became especially challenging in the case of issues that needed quick turnaround time. When we moved to the open spec, we saw that as a great opportunity to move to a more distributed, community-involved model, hoping to avoid the mono-repo issues we had faced for a few years. When determining which languages to bring over, we looked at a variety of data points and usage trends to determine overall language popularity and usage of our existing artifacts (i.e. templates and features).
As we've been discussing, we'd love for you to be a maintainer! We're actively working on this now. Thank you so much for your contributions so far and your interest in maintaining longer term, we're really excited to collaborate with you. @samruddhikhandale @joshspicer if you have any specific feedback on the contents of this Feature, it'd be great to get your review, thank you! |
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 for the contribution, we appreciate it 🎉
Left some comments/thoughts!
# Maybe install curl, gcc, make | ||
for x in curl gcc make; do | ||
which $x > /dev/null || (apt update && apt install $x -y -qq) | ||
done |
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.
It would be better to use a generic snippet to check other Features to do 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.
I don't quite follow this comment, can you clarify?
Thanks!
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 believe @eitsupi is referring to using check_packages
helper function instead of this logic.
https://github.com/devcontainers/features/blob/main/src/docker-in-docker/install.sh#LL86-L91C2
I think it would be nice to use check_packages
in this Feature as well for consistency.
(Would also need to copy apt_get_update
along with check_packages
)
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.
Hrm, I'm not sure that those scripts do exactly what I want to do here. For example, they won't work correctly on a non-debian image where gcc
is already installed.
Additionally, copying functions from one install.sh to another doesn't seem to be a great practice, is there a way to share common script snippets between features?
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'm not sure that those scripts do exactly what I want to do here. For example, they won't work correctly on a non-debian image where
gcc
is already installed.
Note that this repository is focused on debian-based containers at this time. Most Features will only work with debian-based images.
Line 32 in 1449f41
"image": "mcr.microsoft.com/devcontainers/base:ubuntu", // Any generic, debian-based image. |
Additionally, copying functions from one install.sh to another doesn't seem to be a great practice, is there a way to share common script snippets between features?
Comments addressed except one I didn't quite understand. Please take another look. |
So it appears that there is something different about the mcr:ubuntu and mcr:debian images which is causing failures. Is there a good way I can test other than just trying to create a container with that as the base image? |
done | ||
|
||
GHCUP_DIR=~/.ghcup/bin | ||
mkdir -p $GHCUP_DIR |
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 wonder if we should add a new user group haskell
and chown & set sticky bit for this folder .ghcup
Ref: https://github.com/devcontainers/features/blob/main/src/ruby/install.sh#LL206-L209C7 &
https://github.com/devcontainers/features/blob/main/src/ruby/install.sh#L283-L285
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.
where
USERNAME="${USERNAME:-"${_REMOTE_USER:-"automatic"}"}"
We have https://github.com/devcontainers/features/blob/main/src/ruby/install.sh#L39-L59 to automatically figure out the USERNAME
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'm not sure it's necessary, this will land in the home directory, but it doesn't really need a different username imho.
@samruddhikhandale comments mostly addressed except the creating a new I'm open to a more standard package install solution if it is 100% required, but it seems not super useful to just cut and paste the same thing into multiple locations. Additionally, I think we would need to edit it so that it doesn't require a But I'm motivated to get this merged :) so if those changes are required, let me know and I will make them. |
I don't think there is any chance at all that this repository will merge this PR. Lines 98 to 111 in 1449f41
So the standard practice is for you to create your own repository and manage it there. But if you would prefer to manage this in a multi-person organization rather than managing this on your own, @jcbhmr and I are discussing in https://github.com/orgs/devcontainers-community/discussions/1 about creating such a repository. (I am not sure if such a repository or organization will really materialize.) |
Cross ref microsoft/vscode-dev-containers#1705 |
@brendandburns Would it be worth reaching out to https://github.com/haskell somehow? Maybe via https://github.com/haskell/meta to ask if they can add your code to like a haskell/devcontainer-features or similar repo? I know there has been some discussion about what to do about non- /cc @eitsupi |
Thanks so much everyone for the brainstorming and reviews here- I really appreciate everyone providing their feedback and collaborating to find the best path forward. @brendandburns is joining us as a spec maintainer, and the thinking here is his direct spec involvement, plus long-standing container and Haskell expertise, allows the spec to host this Feature (avoids the mono-repo / language expertise issues of vscode-dev-containers). I also think the suggestion to work with Haskell to host the Feature directly is quite interesting - I really appreciate folks sharing that idea. I've opened this issue in the Haskell repo: haskell/meta#4, and we'll continue sharing updates in this PR. |
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.
Looks great, thank you so much! ✨ ✨
|
||
export GHCUP_INSTALL_BASE_PREFIX=${_REMOTE_USER_HOME} | ||
|
||
${GHCUP_DIR}/ghcup install ghc $BOOTSTRAP_HASKELL_GHC_VERSION |
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 suggest to not do all this manually, but use the bootstrap script, which allows fine grained control via env vars.
E.g.
curl --proto '=https' --tlsv1.2 -sSf https://get-ghcup.haskell.org | BOOTSTRAP_HASKELL_NONINTERACTIVE=1 BOOTSTRAP_HASKELL_CABAL_VERSION=$CABALVERSION BOOTSTRAP_HASKELL_INSTALL_HLS=1 BOOTSTRAP_HASKELL_ADJUST_BASHRC=1 sh
This will also try do adjust shell config to add ~/.ghcup/bin
to PATH and most importantly install stack installation hooks that make sure HLS actually works with stack (there are ABI issues). See https://www.haskell.org/ghcup/guide/#strategy-2-stack-hooks-new-recommended
Right now the bootstrap script assumes you want latest of stack and HLS, but I can add environment variables to control that as well.
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.
Oh and the bootstrap script also creates a ~/.ghcup/env
file that can be sourced to get the correct PATH.
set -e | ||
set -o xtrace | ||
|
||
GHCUP_VERSION="0.1.19.2" |
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.
Yeah, please use the bootstrap script to always get the latest ghcup. Pinning ghcup version is not recommended.
|
||
# Definition specific tests | ||
check "version" ~/.ghcup/bin/ghc --version | grep 9.2.6 | ||
|
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.
So is ~/.ghcup/bin
in PATH?
"options": { | ||
"version": { | ||
"type": "string", | ||
"proposals": [ |
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'm not too familiar with how features work. My feeling is that this list can become outdated quickly. For HLS, it already is. Would it not be enough to choose between recommended and latest? Can the user enter manual strings regardless?
No description provided.