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

Windows workflow broken since recent commit #245

Closed
essen opened this issue Jan 26, 2024 · 20 comments · Fixed by #246 or #251
Closed

Windows workflow broken since recent commit #245

essen opened this issue Jan 26, 2024 · 20 comments · Fixed by #246 or #251
Labels
bug Something isn't working

Comments

@essen
Copy link

essen commented Jan 26, 2024

The bug

Starting from this commit: afb8586

The command erl can no longer be found from within the workflow (using msys2). It was working fine the commit just before that.

It appears that the INSTALL_DIR_FOR_OTP has changed in this commit, might be related:

INSTALL_DIR_FOR_OTP: D:\a\_temp\.setup-beam\otp                                  # before
INSTALL_DIR_FOR_OTP: C:\hostedtoolcache\windows\otp\windows-2022\26.2.1\x64      # after 

Software versions

https://github.com/ninenines/ci.erlang.mk/blob/master/.github/workflows/ci.yaml#L99

(used in Cowboy for example)

How to replicate

You can fork Cowboy, enable actions in the fork (note that the action is scheduled so it needs to be enabled in two places), then run them. Then fork ci.erlang.mk and change the setup-beam version (as shown above), and update your Cowboy fork to use the forked ci.erlang.mk to make it fail.

Expected behaviour

Tests running!

@essen essen added the bug Something isn't working label Jan 26, 2024
@paulo-ferraz-oliveira
Copy link
Collaborator

I'll look at this from a test point of view. I was under the impression we were testing the existence of the binaries from the env. variables, but maybe we're just making sure they're issued 🤷. In the meantime, it's probably best you use the complete version, as suggested in the README.

@paulo-ferraz-oliveira
Copy link
Collaborator

Yeah, our CI only makes sure the env. variables are issued, not that they actually work. I'ma try to have CI test that too, for completeness.

@paulo-ferraz-oliveira
Copy link
Collaborator

paulo-ferraz-oliveira commented Jan 28, 2024

https://github.com/erlef/setup-beam/actions/runs/7687124838/job/20946801758 is failing, as expected. I'll now push until I get a proper fix in (I think the culprit is the action not adding bin to the cache folder before exposing the env. variables)

@paulo-ferraz-oliveira
Copy link
Collaborator

Almost there: https://github.com/erlef/setup-beam/actions/runs/7687149859/job/20946858603. Working for all except rebar3. Works for Ubuntu, though.

@paulo-ferraz-oliveira
Copy link
Collaborator

This should now be good: https://github.com/erlef/setup-beam/actions/runs/7687232275/job/20947039524. I'll open a pull request shortly. In the meantime you can try the action with this branch and let us know if you have any more related issues.

Ref: erlef/setup-beam@fix/bin-folder-var-refs

Thanks for the report.

@paulo-ferraz-oliveira
Copy link
Collaborator

Now merged, so you can also use reference erlef/setup-beam@main. I'll wait a while for your comments and then close this and release a patch, under v1.y.z.

@essen
Copy link
Author

essen commented Jan 28, 2024

Hello, unfortunately that doesn't seem to fix it. See https://github.com/ninenines/cowboy/actions/runs/7669467476/job/20948094192#step:5:52 that uses erlef/setup-beam@fix/bin-folder-var-refs

@paulo-ferraz-oliveira
Copy link
Collaborator

How are you executing the binary? Simply concatenating the env.var value with erl, and executing that? Or something else? I see it fail, but I don't know your whole CI.

@paulo-ferraz-oliveira
Copy link
Collaborator

Hm... I'm gonna check what executables I'm calling, since I'm probably missing something here. CI's Ok but that doesn't mean (as you point out) it's Ok for your use case.

@paulo-ferraz-oliveira
Copy link
Collaborator

paulo-ferraz-oliveira commented Jan 29, 2024

From your output I think I see what's happening. The env. variable doesn't contain the /bin (so now your CI is doing /bin/bin). I'll need to fix that and add that information to README.md. I've also updated CI so we use powershell and the same binaries consumers will most likely use too (the .exe, .ps1, etc., for Windows).

Edit: this time I'm pushing it to branch fix/bin-env-vars and not opening a pull request until I have an Ok in this thread. The previous change (while not according to previous behaviour) is, at least, still consistent.

Edit: something like this perhaps?

@essen
Copy link
Author

essen commented Jan 29, 2024

The env var is added to the PATH with /bin appended and then just erl is called.

I just launched a re-run with the fix/bin-env-vars branch and will report the results. Thank you!

@essen
Copy link
Author

essen commented Jan 29, 2024

Unfortunately it doesn't work. I think it has to do with moving the executable from D: to C:.

I did a run to demonstrate the problem: I can call the executable explicitly but I cannot call it from the path: https://github.com/ninenines/cowboy/actions/runs/7694756322/job/20968719461#step:5:13

I have added the explicit cygpath -m conversion to make sure the Windows style path wasn't causing issues but normally it isn't necessary so you may disregard it.

I have vague recollection of running into this a long time ago (not being able to run executables from a different disk) but not remembering enough to understand why or how to fix it adequately.

I suppose an option to be able to install Erlang on D: would be good enough for my purposes, if that's the issue.

@paulo-ferraz-oliveira
Copy link
Collaborator

paulo-ferraz-oliveira commented Jan 31, 2024

Lemme look even closer at this 🤔 I know nothing about this "C: v. D:" stuff, so need to read on it.

Thanks for the details and the example run, they'll help. 👍

Our goal is not to break anything, so if it was in D: it's probably where it should be still (if it's breaking for you it might break for other consumers - I don't know how common your use case is).

I need to check what changed, though. If I remember correctly - I did the most recent changes to the cache stuff - I just reuse whatever paths I'm given by the tools, I don't choose the drives or anything like it. And thanks for being patient, too.

Edit:

I suppose an option to be able to install Erlang on D:

We don't control where stuff gets installed by GHA, we're using their tool-cache lib., but I'll check if there're options.

@essen
Copy link
Author

essen commented Jan 31, 2024

No worries, I pinned the most recent working commit for the time being, and I'm in no hurry.

GHA comes with msys2 pre-installed so if they broke something related to msys2 it's possible they're not testing for it. But I'm not sure it's a C: vs D: issue, or where the exact issue could come from.

Happy to help test things.

@paulo-ferraz-oliveira
Copy link
Collaborator

It seems the prior install code (for Windows/OTP, at least) ran the installer in the context of runner filesystem (not the cache system's). Since stuff has changed a bit (no more scripts, for example) and to keep consistency between tools, I'll try to "copy cache to runner" before install (which should effectively move C: to D:). If all goes well, the initial results are restored, i.e.: INSTALL_DIR_FOR_OTP: D:\a\_temp\.setup-beam\otp

@paulo-ferraz-oliveira
Copy link
Collaborator

@essen, stuff's now in D:, as per https://github.com/erlef/setup-beam/actions/runs/7732061720/job/21081124295#step:5:9. Wanna try the branch (fix/bin-env-vars) out again? 👍

@essen
Copy link
Author

essen commented Jan 31, 2024

OK triggering a build and will check the results tomorrow.

@essen
Copy link
Author

essen commented Jan 31, 2024

Everything seems to work fine now: https://github.com/ninenines/cowboy/actions/runs/7697642536/job/21083084161#step:4:15

The install went to D: and the tests succeeded. So it appears that this C: / D: was really the issue. Not sure why it works like that but happy enough.

@paulo-ferraz-oliveira
Copy link
Collaborator

Perfect, than I'll move the pr from draft, and once it's reviewed and merged I'll release 1.17.3. Thanks.

@paulo-ferraz-oliveira
Copy link
Collaborator

Released in v1.17.3. v1 and v1.7 adjusted to that commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants