Specifying resolver in stack-arguments

[keszocze]

When specifying stack-arguments: --resolver lts-19.33, the github action outputs

Run tmp=$(mktemp)
actual: ghc-9.4.5
wanted: ghc-9.4.5

Scrolling through the log, I can see that the correct GHC version is used (Preparing to download ghc-tinfo6-9.0.2 ...) in the build.

The steps stack-query and stack-path do not pass ${{ inputs.stack-arguments }} to the call to stack. This does produce incorrect information, if I am not mistaken.

I am also wondering whether the "Dependencies" step works correctly (I am new to stack as well as to github actions). The corresponding run code is

    run: |
      stack --no-terminal --stack-yaml ${{ inputs.stack-yaml }} \
        ${{ steps.setup.outputs.resolver-nightly }} \
        setup
      stack --no-terminal --stack-yaml ${{ inputs.stack-yaml }} \
        ${{ steps.setup.outputs.resolver-nightly }} \
        build --dependencies-only --test --no-run-tests \
        ${{ inputs.stack-arguments }}

The setup sub-command is using a different resolver than the build command in case a resolver was explicitly provided in stack-arguments. In the setup step, GHC 9.4.5 is downloaded but then the build step uses the correct version:

 Cache not found for input keys: lts-19.33/Linux-stack [...]
Preparing to install GHC (tinfo6) to an isolated location. This will not interfere with any
system-level installation.
Preparing to download ghc-tinfo6-9.4.5 ...
[...]
Installed GHC.
Stack will use a sandboxed GHC it installed. To use this GHC and packages outside of a project,
consider using: stack ghc, stack ghci, stack runghc, or stack exec.
Preparing to install GHC (tinfo6) to an isolated location. This will not interfere with any
system-level installation.
Preparing to download ghc-tinfo6-9.0.2 ...
[...]
Installed GHC.
[1 of 2] Compiling Main             ( /home/runner/.stack/setup-exe-src/setup-6HauvNHV.hs, /home/runner/.stack/setup-exe-src/setup-6HauvNHV.o )

Will caching work as I intend when adding cache-prefix: lts-19.33/ to the action (as seems to be the correct way according the this actions's workflow file?)

[pbrisbin]

Yeah, you're right that stack-arguments is a bit broken, but I haven't thought of the best way to address it yet.

Originally, it was meant to represent arguments to build specifically (think --no-test), but of course you can put whatever you want there, so "global" arguments (like --stack-yaml or --resolver) are possible, starting getting used, and thus we had to ensure we pass these arguments to all commands -- but often get it wrong.

So we should probably ensure we supply them to all stack invocations. I just wonder if something needed for build is nonsensical for setup or query (or vice-versa) and could cause an error. To correct for that, we'd need to expand our inputs into global-vs-build-vs-{whatever subcommand}-arguments, I think, which just sounds really painful. Or make distinct inputs (like resolver) and sign up for the maintenance burden of matching all stack usage we care about, across all versions we support, forever, which is also terrible...

[deemp]

I think it's enough to have stack-build-arguments, stack-query-arguments, stack-setup-arguments, stack-setup-arguments inputs that are '' by default and use stack-arguments as a fallback value.

Something like this with ternary expressions.

${{ inputs.stack-build-arguments != '' && inputs.stack-build-arguments || inputs.stack-arguments }}

[pbrisbin]

I like that. Would you like to make a PR?

[deemp]

Yes. What's the purpose of --resolver nightly? Why is it set to just nightly? A resolver may be specified in stack-nightly.yaml.

stack-action/action.yml

Lines 131 to 133 in ac0dd8d

	if ! has_resolver && [[ "${{ inputs.stack-yaml }}" == 'stack-nightly.yaml' ]]; then
	resolver_nightly='--resolver nightly'
	fi

[deemp]

If we have specific stack-build-arguments and stack-test-arguments, should we keep separate fast and pedantic inputs?

stack-action/action.yml

Lines 12 to 19 in ac0dd8d

	fast:
	description: "Pass --fast in build/test"
	required: true
	default: true
	pedantic:
	description: "Pass --pedantic in build/test"
	required: true
	default: true

They remove some duplication in common cases, but may conflict with stack-build-arguments. E.g., if a person specifies stack-build-arguments: --fast and doesn't specify fast, stack build --fast --fast will fail.

[pbrisbin]

What's the purpose of --resolver nightly?

You can only set a specific nightly in stack.yaml (e.g. resolver: nightly-2023-02-01), so unless you want your scheduled build, that you intend to run against the latest nightly every day, to be forever stuck on that specific date by mistake, you have to pass --resolver nightly.

should we keep separate fast and pedantic inputs?

I could be convinced either way, but I think so. I think the de-duplication is a better trade-off than the risk of any edge-case.

if a person specifies stack-build-arguments: --fast and doesn't specify fast, stack build --fast --fast will fail.

I think this might be a typo, I think you mean does specify fast? That's the only way I can see --fast --fast resulting.

In other words, if the edge-case that breaks is a user specifying:

with:
  fast: true
  stack-build-arguments: --fast

Or

with:
  pedantic: true
  stack-test-arguments: --pedantic

I think it's reasonable that that fails and we just say, please just do one or the other.

[deemp]

Sorry for a delay.

Okay, got it about nightly and fast and pedantic inputs.

I think this might be a typo, I think you mean does specify fast? That's the only way I can see --fast --fast resulting.

fast is true by default. Though it's required: true, it's not truly required. In other words, a check on the action side is necessary to ensure the input is provided.

I think a warning in the docs should be provided to not specify --fast and --pedantic without specifying fast: ... and pedantic: ....

[pbrisbin]

Maybe fast should be false by default? Then it would all make sense with what I was thinking. Only downside is that's probably a major version bump as it's a break in (default) behavior.

Otherwise, I agree with a warning in the documentation. Though I would probably have it say that one simply should never add --fast or --pedantic, and rely on fast and pedantic only. We could also mention that the options then get used uniformly for all steps so you don't have any accidental recompilation by (e.g.) including --fast on build but not test, and this is why it's preferred.

[deemp]

In #35 (I refer to this commit), there is the stack-build-arguments input that is passed to all stack build invocations and can't be overriden by other inputs.

Setting the default value of stack-build-arguments to --fast --pedantic is equivalent to setting fast: true and pedantic: true.

I think that dropping fast and pedantic inputs will simplify things.

Of course, this change will require a major version bump.

[pbrisbin]

there is the stack-build-arguments input that is passed to all stack build invocations and can't be overriden by other inputs.

Ah, I don't think I understood this. I thought stack-build-arguments was just around for backwards compatibility. I.e. you could use that or the stack-build-arguments-x options. But it sounds like you are saying that the build-arguments and build-arguments-x options are both used.

Something like,

stack-yaml:
  default: "stack.yaml"

stack-arguments:
  description: global arguments to all `stack` invocations, in addition to, --stack-yaml, and --resolver nightly
  default: ""

stack-build-arguments:
  description: arguments passed to all `stack build` invocations
  default: "--fast"

stack-build-arguments-test:
  description: arguments passed to the `stack build --test` invocation
  default: ""

Then, for example, the Test step would be:

- name: Test
  run: |
    stack \
      ${resolver-nightly-if-necessary} \
      --stack-yaml ${inputs.stack-yaml} \
      ${inputs.stack-arguments} \
      build ${inputs.stack-build-arguments} \
      --test ${inputs.stack-build-arguments-test}

Do I have that right?

Of course, this change will require a major version bump.

Yeah, I think the more we look at this, the more I'm finding we'll end up with a bump no matter what. So we might as well rethink it from scratch.

[deemp]

Do I have that right?

Not exactly.

I modified the meaning of stack-build-arguments and should have mentioned that in my previous message.

So, we have:

stack-arguments:
    description: "Additional arguments for stack invocations"
stack-build-arguments:
    description: "Additional arguments in all `stack build` invocations. Can not be overriden by other action inputs."
stack-build-arguments-default:
    description: "Additional arguments in `stack build` invocations. Can be overriden by other action inputs."
stack-build-arguments-dependencies:
    description: "When a non-empy string, overrides the input `stack-build-arguments-default` on the `Dependencies` step."

• stack-arguments are always passed to stack before a command.
• stack-build-arguments are always passed to all stack build commands.
• stack-build-arguments-default are passed to stack build commands unless there is a non-empty input for a particular step.
  • For example, if there is stack-build-arguments-dependencies: --cabal-verbose, on the Dependencies step, stack-build-arguments-dependencies will be passed instead of stack-build-arguments-default.
  • At the same time, stack-build-arguments will also be passed to stack build.

Here's a part of the PR:

# Setup step

echo "stack-build-arguments-build=${stack_build_arguments[*]} ${{ inputs.stack-build-arguments }} ${{ inputs.stack-build-arguments-build != '' && inputs.stack-build-arguments-build || inputs.stack-build-arguments-default }}" >>"$GITHUB_OUTPUT"

# Build step

stack ${{ steps.setup.outputs.stack-arguments }} \
  build --test --no-run-tests \
  ${{ steps.setup.outputs.stack-build-arguments-build }}

Should I rename stack-build-arguments to stack-build-arguments-always to make its meaning more explicit?

[pbrisbin]

I think it's OK to call it stack-build-arguments instead of -always, personally. Though I defer to you.

I don't see a lot of value in stack-build-arguments-default. It seems like the complexity it introduces in the workflow definition is not worth the value it's bringing. Do you disagree?

[deemp]

I think it's OK to call it stack-build-arguments instead of -always, personally.

I also like this shorter name.

It seems like the complexity it introduces in the workflow definition is not worth the value it's bringing. Do you disagree?

Yes, I disagree.

1. stack-build-arguments is for arguments that are passed on all three steps.
2. stack-build-arguments-default is for arguments that are passed on two steps.
   • e.g., a person specifies the stack-build-arguments-dependencies input and lets two other inputs use the stack-build-arguments-default value.
3. Other stack-build-arguments-x inputs are for arguments that are passed on one step.

[pbrisbin]

OK, I see.

So, let's say you wanted --fast --pedantic to apply to build and test steps, but only --fast to apply to dependencies, how would you do that?

with:
  stack-build-arguments-default: --fast
  stack-build-arguments-?: --fast --pedantic
  stack-build-arguments-test: --fast --pedantic

[deemp]

A space is (hopefully) not an empty string.

with:
  stack-build-arguments: --fast
  stack-build-arguments-default: --pedantic
  stack-build-arguments-test: ' '

[pbrisbin]

let's say you wanted --fast --pedantic to apply to build and test steps, but only --fast to apply to dependencies

Ah OK. So I'm guessing you meant,

with:
  stack-build-arguments: --fast
  stack-build-arguments-default: --pedantic
  stack-build-arguments-dependencies: ' '

That seems kind of confusing and hard to intuit to me. I also really don't like that you have to use non-empty white-space with special semantics. Even if we built it this way, I would probably tell my team to not do that and do something like this instead:

with:
  stack-build-arguments: --fast
  stack-build-arguments-build: --pedantic
  stack-build-arguments-test: --pedantic

Or,

with:
  stack-build-arguments-dependencies: --fast
  stack-build-arguments-build: --fast --pedantic
  stack-build-arguments-test: --fast --pedantic

This is still 3 options, and it just seem easier to reason about, which is further evidence, IMO, that -default is not worth the complexity.

That said, I guess users are free to do it any of these ways, so we just have to decide if the implementation of -default is worth the complexity to allow that usage to those that want it. I probably wouldn't do it, but if you are writing the PR you get to decide :)

[deemp]

Ah OK. So I'm guessing you meant,

Yep. Sorry for the misleading typo.

That seems kind of confusing and hard to intuit to me.

I looked at the examples and now feel the same. After all, Explicit is better than implicit.

I probably wouldn't do it, but if you are writing the PR you get to decide :)

As a PR writer, I'd like to bring to the project gradual improvements rather than unnecessary complexity.

I've just removed the stack-build-arguments-default input.

[pbrisbin]

This is addressed in @v5. Thanks @deemp for you patient effort on this!