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

Add wkdev-setup-default-clang script #65

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TingPing
Copy link
Member

This allows easily installing and switching between different clang toolchains

This allows easily installing and switching between different clang toolchains
@TingPing TingPing force-pushed the pgriffis/set-default-clang branch from df8d268 to 6947e62 Compare October 24, 2024 23:01
@TingPing TingPing changed the title Add wkdev-set-default-clang script Add wkdev-setup-default-clang script Oct 24, 2024
ln -s "/usr/bin/${command}-18" "/usr/local/bin/${command}"; \
done && ln -s "/usr/bin/lld-18" "/usr/local/bin/ld.lld";
# Convenience symlinks for clang tools, the VSCode extension doesn't find these by default.
# FIXME: Reduce code duplication with `wkdev-set-default-clang`.
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to invoke this script from here, no? Maybe at a later point in the script, but still..

Copy link
Member Author

@TingPing TingPing Oct 25, 2024

Choose a reason for hiding this comment

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

The wkdev-sdk scripts are not in the image. We could copy them and then remove them, feels not great either.

Copy link
Member

Choose a reason for hiding this comment

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

As long as you copy and remove in the same layer, there is no increase an image size -- I agree looks odd, but helps reducing code duplication...

echo ""

# Sanity check versions Ubuntu actually has.
if (( $version < 14 )) || (( $version > 18)); then
Copy link
Member

Choose a reason for hiding this comment

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

I'm a fan of ${foo} over $foo -- can you adapt to keep things consistent wrt to the other scripts?

echo ""

# Sanity check versions Ubuntu actually has.
if (( $version < 14 )) || (( $version > 18)); then
Copy link
Member

Choose a reason for hiding this comment

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

Maybe introduce some minVersion/maxVersion variables in the pre-amble? Makes it easier to find and not forget about the other place that now use 14/18 (help text below)


# Sanity check versions Ubuntu actually has.
if (( $version < 14 )) || (( $version > 18)); then
echo "$version is not a valid value (between 14-18)."
Copy link
Member

Choose a reason for hiding this comment

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

Following @ntrrgc other PR #64 we should use _log_ instead of echo here.


local output_path
if [ "$EUID" -eq 0 ]; then
output_path='/usr/local/bin'
Copy link
Member

Choose a reason for hiding this comment

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

You could either omit the single quotes, or use double-quotes as we do for all the other strings, unless we do need to avoid variable replacments within the strings.

@TingPing TingPing marked this pull request as draft November 4, 2024 20:24
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.

2 participants