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

Enhancements to crystal env #11694

Open
HertzDevil opened this issue Jan 4, 2022 · 11 comments
Open

Enhancements to crystal env #11694

HertzDevil opened this issue Jan 4, 2022 · 11 comments

Comments

@HertzDevil
Copy link
Contributor

I think we should expose the rest of the special constants under the Crystal module plus some others for crystal env (except Crystal::DESCRIPTION, because it can be computed from other strings), for use in external scripts:

  • CRYSTAL_BUILD_COMMIT: corresponds to Crystal::BUILD_COMMIT.
  • CRYSTAL_BUILD_DATE: corresponds to Crystal::BUILD_DATE.
  • CRYSTAL_HOST_TARGET: corresponds to Crystal::Config.host_target, i.e. the default target shown under crystal --version.
  • CRYSTAL_DEFAULT_PATH: corresponds to Crystal::DEFAULT_PATH.
  • CRYSTAL_LIBRARY_PATH: corresponds to Crystal::LIBRARY_PATH.
  • CRYSTAL_DEFAULT_LIBRARY_PATH: corresponds to Crystal::Config.library_path, i.e. the value of CRYSTAL_CONFIG_LIBRARY_PATH this compiler was built with.

Additionally the command should be able to tell whether optional components of the compiler are available:

  • CRYSTAL_HAS_INTERPRETER: is 0 if the compiler was built with -Dwithout_interpreter, is 1 otherwise. Is implicitly empty for Crystal 1.2 or below.
  • CRYSTAL_HAS_PLAYGROUND: like above, but checks for -Dwithout_playground.

Currently the only way to tell whether a compiler binary has interpreter support is to do crystal interactive ??? 2>/dev/null and then inspect the exit code, and for playground support it is even more tedious. Those extra variables would save the hassle.

@straight-shoota
Copy link
Member

I support the idea to expose more information, but I don't think crystal env is the right place for that. That command is about the environment configuration. What you're mentioning here is information about the compiler build. Conceptually, that's what crystal --version is for.

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Jan 5, 2022

crystal env can and should expose information from crystal --version so that users don't have to parse Crystal::DESCRIPTION themselves to extract the rest of the constants currently unavailable.

We could rename env altogether if there is a more appropriate name, but I think env is fine as is.

@straight-shoota
Copy link
Member

I don't disagree on making this information easily accessible and machine readable.

But I think we should keep a difference between showing compiler build information and runtime configuration.
When I want to know which compiler I'm working with, I run crystal --version. When I want to know the local configuration, I run crystal env. I wouldn't want crystal env to show a bunch more data just about the compiler. Compiler info doesn't change as long as I don't switch the executable. But runtime configuration is customizable.

I would be fine with making the proposed variables accessible via crystal env VAR. But I'd strongly advocate to keep the default output for crystal env small in order to reduce noise. We can add options to show all values in a list format, for example crystal env --compiler, crystal env --all.

@HertzDevil
Copy link
Contributor Author

When I know crystal env exists, I would not want to run a different command that does the same things but only for a set of variables that, for some reason, cannot be shown along the 5 we already have.

And to be honest, going from 5 to 13 lines doesn't seem like noise to me. If a script does `crystal env` then it assumes everything under CRYSTAL_* is available to the compiler; otherwise, it would have extracted only the variables needed with `crystal env CRYSTAL_*` directly, which is good practice compared to `crystal env`. if the variables are only printed, one could always pipe the output to a pager. Marking a set of variables as non-default only hampers the user experience here.

We could still add functionality such as crystal env --compiler to show certain categories of the variables. But since the variable names do not contain "compiler" or form a hierarchy, one may again find those categories arbitrarily defined and less useful than simply listing all or the needed variables.

@straight-shoota
Copy link
Member

To me, the critical distinction is that the values we're talking are not variables. They're fixed constants of the compiler binary. That's different from CRYSTAL_PATH, CRYSTAL_CACHE_DIR etc. which are configurable at runtime. Mixing those different concerns up seems rather odd.

I understand the argument about the similar names suggesting handling them together. But it's based on the premise of actually using the proposed scheme of similar names. I'm not committed to that. Compiler build information are not environment variables, so there's no reason to name them like those.

@HertzDevil
Copy link
Contributor Author

These properties are the variables of a previous build. Besides, the real-world semantics of "variable" are irrelevant here; as long as crystal env exists, all keys it accepts are variables with respect to the command.

As a comparison, consider git config --list. It shows the combined variables from the system, global, and local scopes, unless a corresponding scope option is provided. It makes sense because the average user does not care whether or to what extent these variables are configurable by the user or the installation, only that these variables exist. Git provides --show-origin and --show-scope for diagnosis if something goes wrong.

I think this is the user experience crystal env should achieve. It is easier to refer to these key-value pairs as e.g. "properties", than to keep only some of them as "variables" in one sense and decide that the remaining key-value pairs aren't "variables" in that same sense, excluding them from the default command invocation. (In fact, doing the latter means that scripts which use `crystal env` to acquire CRYSTAL_VERSION would be broken, because it is never overridable.)

@asterite
Copy link
Member

asterite commented Jan 7, 2022

I agree with @HertzDevil here: having just a single command sounds better. You usually want to look at some value, and having to remember which command to run to find it isn't ideal. Maybe the issue is that the command is named env. If it were properties or config maybe it wouldn't be a problem. But env isn't that bad either.

@straight-shoota
Copy link
Member

I can't follow the example with git config. Despite being defined in different scopes, all these config variables can be overridden somewhere in local git config, per command line argument etc. There's no relation to compiler build information.

Taking git as an example, the equivalent to what's being discussed here would be git --version --build-options.

$ git --version --build-options
git version 2.34.1
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh

That fits exactly with my argument for separation.

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Jan 7, 2022

sizeof-long is not part of Git's domain logic, and would not be queried by end users of Git in the same capability as e.g. core.autocrlf. CRYSTAL_HAS_INTERPRETER and CRYSTAL_PATH will be used similarly even though one is overridable and the other is not. That's why they should be primarily exposed by crystal env, not crystal --version.

(For that matter, our sizeof-long can technically be inferred from CRYSTAL_HOST_TARGET; being a compiler property, it definitely bears more relevance in Crystal than it does in Git, so CRYSTAL_HOST_TARGET should belong to crystal env.)

@rubyFeedback
Copy link

rubyFeedback commented Jan 11, 2022

straight-shoota wrote:

To me, the critical distinction is that the values we're
talking are not variables. They're fixed constants of
the compiler binary. That's different from CRYSTAL_PATH,
CRYSTAL_CACHE_DIR etc. which are configurable at runtime.
Mixing those different concerns up seems rather odd.

I am not involved in the main discussion as such, although I
kind of agree with the threadstarter more than with
straight-shoota. But I have no real qualms either way.

But what I did want to point out is ruby's "gem env" output.

It is best to copy/paste it first here when I run it as the
superuser:

RubyGems Environment:
  - RUBYGEMS VERSION: 3.3.4
  - RUBY VERSION: 3.1.0 (2021-12-25 patchlevel 0) [x86_64-linux]
  - INSTALLATION DIRECTORY: /root/.gem
  - USER INSTALLATION DIRECTORY: /root/.gem/ruby/3.1.0
  - RUBY EXECUTABLE: /home/Programs/Ruby/3.1.0/bin/ruby
  - GIT EXECUTABLE: /usr/bin/git
  - EXECUTABLE DIRECTORY: /root/.gem/bin
  - SPEC CACHE DIRECTORY: /root/.gem/specs
  - SYSTEM CONFIGURATION DIRECTORY: /home/Programs/Ruby/3.1.0/etc
  - RUBYGEMS PLATFORMS:
     - ruby
     - x86_64-linux
  - GEM PATHS:
     - /root/.gem
     - /root/.gem/ruby/3.1.0
     - /home/Programs/Ruby/3.1.0/lib/ruby/gems/3.1.0
  - GEM CONFIGURATION:
     - :update_sources => true
     - :verbose => true
     - :backtrace => false
     - :bulk_threshold => 1000
  - REMOTE SOURCES:
     - https://rubygems.org/
  - SHELL PATH:
     - /usr/bin/
     - /usr/sbin/
     - /bin/
     - /sbin/
     - /usr/local/bin/
     - /opt/qt/bin/
     - /opt/java/bin/
     - /usr/local/texlive/2019/bin/x86_64-linux/
     - /root/.cargo/bin
     - /root/.local/bin

While I can understand straight-shoota's position, I would
like to point out that at the least "gem env" shows additional
information that is user-amenable. Things such as $SHELL PATH
but also I believe some other paths (some ruby env variables
that can be set by the user). So the comment about "fixed
constants" as primary rationale is, in my opinion, not a
great one necessarily IF users can influence/modify them.
In which case the primary question, then, should be what
is USEFUL to the user pertaining to the environment and
possible problems due to any faulty/flawed environments.
(For instance, I once set the variable TZ to be a shortcut
for .tar.gz, but when I did this all timezone-related things
broke, because TZ or whatever it was, was taken by
glibc or whatever is responsible for the timezone-related
stuff, and it took me an hour or two to understand this
finally ... shell scripts can be a real pain ...)

See python's pip/virtualenv for a related consideration
one may include. So, the TL;DR version:

I believe the user's consideration, what the user needs
or wants and may consider useful, should be the primary
focus on discussion, not the "perfect cleanliness" in
regards to "this is a constant, nothing may be changed
and output must be limited/short".

Perhaps if you'd want to you could add some additional
flag to "env" or add a "gem envv" command - the extra
"v" would indicate "extra verbosity". :D (Or simply
change the default as-is, as was suggested by the
threadstarter, which I think may be useful too.)

@Fryguy
Copy link
Contributor

Fryguy commented Jan 11, 2022

One other argument for the single command is in diagnostics. If something goes wrong, we can just ask the user to run crystal env and then copy-paste everything (I think this is the same reasoning that @rubyFeedback is suggesting as well).

Thinking about @asterite's comment that maybe the env bit is what we're hung up on, if the primary purpose is diagnostics, perhaps crystal doctor is a better command (similar to brew doctor)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants