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

Added std lib files into installer #640

Open
wants to merge 6 commits into
base: staging
Choose a base branch
from

Conversation

KrosFire
Copy link
Member

Hello there,

For the LSP I need std lib files on the client machine, and not as a part of the compiler. I didn't touch Nix files as it is scientifically proven fact that it is black magic.

I think there can be a lot of edge cases, so any tests will be most welcome.

Enjoy.

@KrosFire KrosFire self-assigned this Dec 20, 2024
snap/snapcraft.yaml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
setup/shared.ab Outdated Show resolved Hide resolved
src/stdlib.rs Outdated Show resolved Hide resolved
@Ph0enixKM Ph0enixKM added enhancement New feature or request compiler labels Dec 21, 2024
snap/snapcraft.yaml Outdated Show resolved Hide resolved
snap/snapcraft.yaml Outdated Show resolved Hide resolved
src/stdlib.rs Outdated Show resolved Hide resolved
@soumyaDghosh
Copy link
Contributor

2 more things @Ph0enixKM I'd like to brirng in your account. I think the project needs a rename from amber to amber-bash, because

  1. You registered the snap name under amber-bash which was a blessing in disguise.
  2. Another project on Github has a copyright of the name amber from 2012, (not an expert of such things, so, better to have some more expert opinions on this)
  3. When one needs to run the snap, he'll need to run it as amber-bash not amber. And amber-bash gives this output, which is misleading and needs to be fixed
Usage: amber [OPTIONS] [INPUT] [ARGS]... [COMMAND]

Commands:
  eval        Execute Amber code fragment
  run         Execute Amber script
  check       Check Amber script for errors
  build       Compile Amber script to Bash
  docs        Generate Amber script documentation
  completion  Generate Bash completion script
  help        Print this message or the help of the given subcommand(s)

Arguments:
  [INPUT]    Input filename ('-' to read from stdin)
  [ARGS]...  Arguments passed to Amber script

Options:
      --no-proc <NO_PROC>  Disable a postprocessor
                           Available postprocessors: 'shfmt', 'bshchk'
                           To select multiple, pass multiple times with different values
                           Argument also supports a wilcard match, like "*" or "s*mt"
  -h, --help               Print help
  -V, --version            Print version

I initially thought of alias, but due to this copyright, I am not sure we'll be able to get an alias to amber.

@Ph0enixKM
Copy link
Member

Ph0enixKM commented Dec 21, 2024

@soumyaDghosh This project that you shared is not being maintained 7 years now. It seems that it's something that they abandoned long time ago. I think that there is no problem with us sticking to the name "amber".

Quite frankly anyone could name their project "amber". But unless this name is not widely recognized in the shell scripting environment, I'd say it's still a good one.

You've mistaken copyright for trademark, @soumyaDghosh

@Ph0enixKM
Copy link
Member

How can we create an amber alias for amber-bash?

@soumyaDghosh
Copy link
Contributor

How can we create an amber alias for amber-bash?

You just create a forum post requesting an alias, similar to this

Users can anyways create local aliases,

sudo snap alias amber-bash amber

@KrosFire
Copy link
Member Author

KrosFire commented Dec 21, 2024

I've decided that there are too many installer variations. It needs to be unified. Cargo dist is my pick as it compiles for macos, linux and windows on all architectures and offers easy updates. Packaging is also simple. I added config just for shell installer but homebrew is also an option

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this file deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Like I said before, I think that having many different installers is going to be cumbersome for both users and devs. They won't know what's the correct way to install Amber and uninstall it. There can be a situation where one persons has installed amber in three different ways on his/hers machine. One install script to rule them all is the best way to go about it (I think)

@Ph0enixKM
Copy link
Member

@KrosFire btw since Amber is already installed, can we write uninstaller in Amber itself?

@KrosFire
Copy link
Member Author

@KrosFire btw since Amber is already installed, can we write uninstaller in Amber itself?

You're right

@Ph0enixKM
Copy link
Member

I love this uninstall sentence "see you later 🐊"

Copy link
Member

@b1ek b1ek left a comment

Choose a reason for hiding this comment

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

i don't get it. is there some kind of mistake with committing wrong files? why are so many things deleted? how are any of those deletions relevant?

please explain why you had to get rid of all those files, and how will it help you with putting stdlib into the installer. especially since you deleted the installer in question

@b1ek
Copy link
Member

b1ek commented Dec 22, 2024

i strongly suggest that we discuss the change itself in #643 , and reserve this space for code review

Copy link
Contributor

@hdwalters hdwalters left a comment

Choose a reason for hiding this comment

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

Leaving comments only, as I do not feel qualified to review installer changes.

permissions:
contents: write
"contents": "write"
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems unnecessary to add quotes around either name or value here. There may be specific reasons why it's necessary, but YAML itself has no such requirement.

plan:
runs-on: ubuntu-latest
runs-on: "ubuntu-20.04"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to target a specific version of Ubuntu, especially such an old one? (two LTS releases ago!)

Copy link
Member

@Ph0enixKM Ph0enixKM Dec 25, 2024

Choose a reason for hiding this comment

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

This was generated by cargo dist. I checked their issue on updating the runner to the LTS latest, but their reasoning was to further improve compatibility. I don't think that we'd benefit from it. I'd advise to go away from cargo dist and build our own CI with cargo cross (a cross platform compiler) the way we do for linux-arm right here

@@ -0,0 +1,12 @@

let amber_path = $command -v amber$ failed {
Copy link
Contributor

@hdwalters hdwalters Dec 23, 2024

Choose a reason for hiding this comment

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

Please:

  1. Don't leave blank lines at top of file.
  2. Consider adding a shebang line and making the file executable.
  3. Consider moving Amber code to a main block.
  4. Add inner spaces in commands $ ... $, as $command looks like a Bash variable.
  5. Rename amber_installation_dir to something shorter.
#!/usr/bin/env amber

main {
    let amber_path = $ command -v amber $ failed {
        echo "Amber is not in $PATH."
        exit 1
    }

    let amber_dir = trust $ dirname "\$(readlink -f {amber_path})" $

    echo "Uninstalling Amber..."
    trust $ rm -f {amber_path} $
    trust $ rm -rf {amber_dir} $
    echo "Amber uninstalled. See you later, 🐊"
}

use std::{env, fs, path::PathBuf};

#[cfg(not(debug_assertions))]
fn get_install_dir() -> PathBuf {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why you need different versions of this function for debug and release builds. Consider picking one and using it for both.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, please move private functions below public function resolve(), which they support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants