Fix windows executable name

[jeremyvaartjes]

The windows version of this setup script was copying the executable without the .exe extension so I'm adding a function that works out what the file name should be for the given os.

[StefMa]

I am not a windows user nor do I need to run something on a windows machine, so I ask myself if this is really necessary on github actions to suffix it with exe? 🤔
Is it not working without the prefix? Or is it just the "more correct way"?

[jeremyvaartjes]

So if you omit the .exe on windows, it will treat it as a file to be opened in another program. The .exe tells windows that it is an executable. I've double checked this within powershell which is the underlying shell that github actions uses.

[jeremyvaartjes]

Funnily enough windows does have metadata within the file to also say that it is an executable but choses not to use it in cases like this 🤷

[jeremyvaartjes]

This is an example of running pkl --version directly after the setup pkl command and it doesn't report the version. My testing indicates that powershell reports a successful exit code (which is why it doesn't fail in github actions) and my suspicion is that on a GUI version of windows it shows a "select an app to open 'pkl'" prompt and that executable runs successfully.

[jeremyvaartjes]

If you check this build here in the "Confirm download" step, it doesn't actually run the command
https://github.com/pkl-community/setup-pkl/actions/runs/10458702810/job/28961105461

[StefMa]

Thanks for this reasearch @jeremyvaartjes !
This is super intersting.

However, how can npm for example run on a windows machine? 🤔
I have another project where I do this:

  test:
    strategy:
      matrix:
        os: [macos-13, macos-14, ubuntu-latest, windows-latest]
    runs-on: ${{ matrix.os }}
    steps:
      - name: Checkout
        uses: actions/checkout@v4
      - uses: actions/setup-node@v4
        with:
          node-version: '20'
      - run: npm install
      - run: npm test

Here, npm install and npm test will also be executed on windows.
Why isn't a .exe required here? 🤔

[StefMa]

Alright, I tested a bit here and loked what the setup-node project is doing.
Basically, it saved the node.exe (or normal node) inside a temp directoy, and cache that dir (instead of a file).
See https://github.com/actions/setup-node/blob/26961cf329f22f6837d5f54c3efd76b480300ace/src/distributions/base-distribution.ts#L211-L264

When we map to this the setup-pkl project, we can do something like this (hacked in, not nice 😅 ):

    const pklCacheDir = path.join(process.env['RUNNER_TEMP']!!, 'tmp_pkl')
    await io.mkdirP(pklCacheDir)
    if (os.platform() === 'win32') {
      await io.cp(pklBinaryPath, path.join(pklCacheDir, 'pkl.exe'))
    } else {
      await io.cp(pklBinaryPath, path.join(pklCacheDir, 'pkl'))
    }
    const cachedPath = await tc.cacheDir(pklCacheDir, 'pkl', pklVersion)

With that, I made it to work running just pkl on all github hosted runners:

pkl.mov

Maybe this is a better idea than using pkl.exe on windows runners, while using just pkl on macos and linux runners.
Also because (I guess) all other setup- actions are doing the same and omitting the .exe extension.

What do you think?

[jasongwartz]

Thanks for the extensive research both, much appreciated!

Also because (I guess) all other setup- actions are doing the same and omitting the .exe extension.

This makes sense to me, happy to go with the implementation as researched by @StefMa.

If you check this build here in the "Confirm download" step, it doesn't actually run the command

This is moderately concerning. I know basically zero about Windows and Powershell, is there a better way to detect a failure of the executable @jeremyvaartjes ?

[jeremyvaartjes]

Powershell would output an error if the file didn't exist or was named differently, its only because the name matches that it decides "I want to open this as a file".
The main way I think you could possibly get around this is checking the output for an expected output. eg: pkl --version would output a known version string for a given version so you could just match the output to ensure that the actual pkl application is running. I guess the only thing with this is that it only works if the pkl executable isn't changing versions all the time, otherwise you would have to update the match string each time.

[jeremyvaartjes]

Apparently the start process command allows running without the file extension because it takes in an explicit file path, but then if you start using that method you are again creating a bunch of windows only code to get very little value. It's also a problem for all downstream users since they would need to use the same workarounds.

[jeremyvaartjes]

I think @StefMa's suggested change and my change have probably the same level of windows specific code. I think @StefMa's version might be better for the long run if the pkl team decide to distribute more than a single executable file because his version starts working with directories rather than a singular file.

[jasongwartz]

I've looked at added a test for the output of invoking pkl with: #43

If you check this build here in the "Confirm download" step, it doesn't actually run the command

In the CI runs of #43, the downloads including Windows are executing properly, so now I'm confused about whether or not the issue in this PR is currently a problem?

[jasongwartz]

Oh, now that I've said that - that must be because I've tried using https://github.com/marketplace/actions/command-output to execute the command (and capture output) and it looks like it's defaulting to bash instead of Powershell

[StefMa]

@jasongwartz the action you're using to test it runs on bash.
See https://github.com/mathiasvr/command-output/blob/6309e0ee6ed594fd7f382bd826e138e0ba04dd63/index.js#L66
There it uses the core.getInput(shell).
Its default in the action.yml as bash. See https://github.com/mathiasvr/command-output/blob/6309e0ee6ed594fd7f382bd826e138e0ba04dd63/action.yml#L10

The normal windows runner uses powershell as default (according to @jeremyvaartjes).
But it seems it also supports bash as a shell 🤷

So still, the error exist.

I think @StefMa's version might be better for the long run if the pkl team decide to distribute more than a single executable file because his version starts working with directories rather than a singular file.

I don't think this is the only valid case for this 😅
I think its also a valid case because for whatever reasons pkl (without the .exe extenion) can be used when cahing the exe in a directoy instead of a file...
And this might be a good reason to do so.

[jeremyvaartjes]

I've made an update to this pr to extract some of the platform logic into it's own file, that will hopefully mean that the main file is kept a bit cleaner.
I've also updated the CI file so that it checks the output of the pkl --version command. Powershell again has a different way of doing things so I've had to use conditional steps.
I've kept with the tc.cacheFile command for now as I think doing the whole directory thing is a bit complex for our needs right now, we can switch it up later.

[jeremyvaartjes]

@jasongwartz Is there anything you want me to adjust or are we good to merge this PR?

[jasongwartz]

I'd like to take another look together at simplifying the implementation. Is this a bugfix that's blocking you on something?

[jasongwartz]

I generally prefer as const objects or arrays instead of enums - enums are one of the few TypeScript features which are transpiled into entirely different runtime code (whereas most TypeScript type-related syntax is simply erased).

[jasongwartz]

Doesn't look like these two are used by the caller

[jasongwartz]

I don't think this can happen here, right? Since arch is of type Architecture, and determineArch() can only return one of the two supported values

[jasongwartz]

Suggested change

	case 'linux':
	switch (arch) {
	case 'aarch64':
	return 'pkl-linux-aarch64'
	case 'amd64':
	return 'pkl-linux-amd64'
	default:
	throw new Error('Unsupported architecture')
	case 'linux':
	return `pkl-linux-${arch}`

Just looking at the logic, would something like this cover the necessary behaviour?

[milesj]

Any movement here?

[jasongwartz]

thanks for the improvement, and sorry for the delay!