Skip to content

artifact: add path/binary/isInstalled static helpers#1169

Open
henderkes wants to merge 1 commit into
v3from
v3c/artifact-static-helpers
Open

artifact: add path/binary/isInstalled static helpers#1169
henderkes wants to merge 1 commit into
v3from
v3c/artifact-static-helpers

Conversation

@henderkes
Copy link
Copy Markdown
Collaborator

Give zig, rust, go_win and go_xcaddy a small consistent surface for locating the install directory and a binary inside it:

  • path(): install/extract root for the artifact
  • binary($name = ''): full path to a binary under that root, picking the artifact's natural layout (top-level for zig, bin/ for rust and the go toolchains)
  • isInstalled(): is the default binary present on disk

Callers that previously concatenated PKG_ROOT_PATH . '/zig/zig' (and the equivalents for the other artifacts) by hand can call the helpers instead, and any later code that needs to ask "is this toolchain available" can use isInstalled() without rebuilding the path.

What does this PR do?

Checklist before merging

  • If you modified *.php or *.yml, run them locally to ensure your changes are valid:
    • composer cs-fix
    • composer analyse
    • composer test
    • bin/spc dev:lint-config

Give zig, rust, go_win and go_xcaddy a small consistent surface for
locating the install directory and a binary inside it:

- path(): install/extract root for the artifact
- binary($name = '<default>'): full path to a binary under that root,
  picking the artifact's natural layout (top-level for zig, bin/ for
  rust and the go toolchains)
- isInstalled(): is the default binary present on disk

Callers that previously concatenated PKG_ROOT_PATH . '/zig/zig' (and
the equivalents for the other artifacts) by hand can call the helpers
instead, and any later code that needs to ask "is this toolchain
available" can use isInstalled() without rebuilding the path.
Copy link
Copy Markdown
Owner

@crazywhalecc crazywhalecc left a comment

Choose a reason for hiding this comment

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

I'll leave this tomorrow. Look for a better way if possible.

@crazywhalecc
Copy link
Copy Markdown
Owner

go-win:
  type: target
  artifact:
    binary: custom
    metadata:
      install-path: go-win
      default-binary: go.exe
      binary-subdir: bin

I prefer setting them in config and using getInstallPath() and other similar functions to get. Soft-defining functions in artifact itself looks not appropriate.

@henderkes
Copy link
Copy Markdown
Collaborator Author

This is something where I don't really see an advantage to describing it in a config. The function definitions are at least as short as and more ergonomic to use.

I'm not terribly concerned with it though, so if you want to change it, I don't object.

@crazywhalecc
Copy link
Copy Markdown
Owner

This is something where I don't really see an advantage to describing it in a config. The function definitions are at least as short as and more ergonomic to use.

I'm not terribly concerned with it though, so if you want to change it, I don't object.

I just thought that we should keep constant things in config, and dynamic things written in PHP functions.

I don't see your parts containing dynamic codes so that I prefer defining in config. Also we could define dynamic things in another parts as you like.

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