Amber icon indicating copy to clipboard operation
Amber copied to clipboard

Added std lib files into installer

Open KrosFire opened this issue 1 year ago • 17 comments

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.

I've modified installation scripts install.ab/uninstall.ab/snapcraft.yaml and cargo-dist release pipeline to include std lib files.

Enjoy.

KrosFire avatar Dec 20 '24 22:12 KrosFire

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.

soumyaDghosh avatar Dec 21 '24 12:12 soumyaDghosh

@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 avatar Dec 21 '24 14:12 Ph0enixKM

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

Ph0enixKM avatar Dec 21 '24 14:12 Ph0enixKM

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

soumyaDghosh avatar Dec 21 '24 15:12 soumyaDghosh

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

KrosFire avatar Dec 21 '24 16:12 KrosFire

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

Ph0enixKM avatar Dec 21 '24 17:12 Ph0enixKM

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

You're right

KrosFire avatar Dec 21 '24 17:12 KrosFire

I love this uninstall sentence "see you later 🐊"

Ph0enixKM avatar Dec 21 '24 18:12 Ph0enixKM

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

b1ek avatar Dec 22 '24 06:12 b1ek

why is there a lot of changes to CI? the description says that it is about moving stdlib onto the client machine

which hasn't been properly discussed, by the way

It's relavent because moving std lib to client machine involves changing installation process. CI is automatically generated by cargo dist.

KrosFire avatar Jan 13 '25 15:01 KrosFire

@b1ek, please explain your concerns about the "irrelevant" parts of code. The snippet you said that are irrelevant from my point of view bring some value like for instance retrieving cached cargo-dist in a pipeline.

Ph0enixKM avatar Jan 20 '25 12:01 Ph0enixKM

@b1ek, please explain your concerns about the "irrelevant" parts of code. The snippet you said that are irrelevant from my point of view bring some value like for instance retrieving cached cargo-dist in a pipeline.

i just don't understand why they have to be included in this PR specifically, as there is no info in the description about them.

they do provide some value, but the description doesn't say anything about them. so that's irrelevant the way i see it

b1ek avatar Jan 25 '25 11:01 b1ek

I think that the Std files shouldn't be in the installer because a user maybe is not interested in the LSP.

The LSP server should include the std itself like other languages does.

Mte90 avatar Jan 27 '25 11:01 Mte90

I think that the Std files shouldn't be in the installer because a user maybe is not interested in the LSP.

The LSP server should include the std itself like other languages does.

But std lib files are included one way or the other. I just change the way they are included as to not have needless redundancy

KrosFire avatar Jan 28 '25 17:01 KrosFire

@b1ek, please explain your concerns about the "irrelevant" parts of code. The snippet you said that are irrelevant from my point of view bring some value like for instance retrieving cached cargo-dist in a pipeline.

i just don't understand why they have to be included in this PR specifically, as there is no info in the description about them.

they do provide some value, but the description doesn't say anything about them. so that's irrelevant the way i see it

Adding std lib files requires changing how we build release files. We do it with cargo-dist that automatically generates ci config. I changed configuration of cargo-dist - which is relevant, that change propagates into changes in CI file. I think this is relevant. Changing manually CI file to look the same as before is excessive imho

KrosFire avatar Jan 28 '25 18:01 KrosFire

We do it with cargo-dist that automatically generates ci config. I changed configuration of cargo-dist - which is relevant, that change propagates into changes in CI file

just put it in the description, to make it clear to everyone what it does and what is it for

b1ek avatar Jan 29 '25 03:01 b1ek

@Mte90 many popular compilers include standard library installed separately (usually under /usr/lib) common example is GCC

Ph0enixKM avatar Jan 30 '25 13:01 Ph0enixKM