Build.py is broken

[UE4SS]

See: https://github.com/UE4SS-RE/RE-UE4SS/actions/runs/9341222220/job/25707745800

[Buckminsterfullerene02]

You don't say... it's literally fixed in #542 can you just look at, approve and merge that please?

[UE4SS]

It's been a lot of pull requests lately, I can't keep up, sorry, I thought narknons recent commits was supposed to fix the script and that your PR was some kind of rewrite for other reasons. That looks like a very comprehensive PR, it needs to be tested, including making a release, probably in a fork to make sure nothing is broken, kind of painful to test, I'll get to it eventually if no one else does.

[Buckminsterfullerene02]

Yes it probably does need to be tested for actual release. Though, is it really necessary, if bitonality plans on changing the release CI/python script between now and the next actual release anyway? So I'd say it only needs to be tested for now on experimental, which it has.

[bitonality]

This would likely have been caught if the changes were PR'ed for review and not blasted through branch protections. Maybe we should consider hard-locking with no bypass or with a break glass, but out of the scope of this issue.

https://github.com/UE4SS-RE/RE-UE4SS/blob/edaf6fc333d22b935868c980a6dde231a8d3d579/tools/buildscripts/build.py#L74-L84

Context: currently the script creates the releases dir and then passes releases/staging_dev as the dst to the above copy_assets_without_mods.

# The context up to this point...
os.mkdir('releases')
src = 'assets'
dst = 'releases/staging_dev'

# Copy entire directory, excluding Mods folder 
for item in os.listdir(src): 
    s = os.path.join(src, item) 
    d = os.path.join(dst, item) 
    if os.path.isdir(s) and item == 'Mods': 
        continue 
    if os.path.isdir(s): 
        shutil.copytree(s, d) # This will create dest dir
    else: 
        shutil.copy2(s, d) # This needs dest dir to be created or will error

shutil.copy2() needs the directory to exist before attempting to copy. If the os.listdir() iterator executes the shutil.copytree() BEFORE shutil.copy2() then no problems will happen. However, our assets folder has Changelog.MD beating CustomGameConfigs/ alphabetically, so most filesystem impls will have the shutil.copy2() running before the shutil.copytree().

Also mentioning that we need to be careful about testing python scripts locally. The github action currently downloads python 3.12.3 where shutil.copy2() actually calls into a special windows module to achieve faster copies, hence the trace we see in the github output:

   File "C:\hostedtoolcache\windows\Python\3.12.3\x64\Lib\shutil.py", line 460, in copy2
    _winapi.CopyFile2(src_, dst_, flags)
FileNotFoundError: [WinError 3] The system cannot find the path specified

Older python versions do not have the faster system specific shutil implementations and actually return DIFFERENT error codes.

  File "C:\Python39\lib\shutil.py", line 264, in copyfile
    with open(src, 'rb') as fsrc, open(dst, 'wb') as fdst:
FileNotFoundError: [Errno 2] No such file or directory: '/release/staging_dev\\Changelog.md'

Ideally we lock the version of python in the runner to not automatically pull latest and ensure that any local testers have the identical version before testing changes to python scripts.

[narknon]

Actually, the script was majorly broken by a different PR that was approved and merged before I could re-review it after requesting changes. So, unlikely anything would have been caught by the approval of a PR for my commit. My initial commit worked fine until I tried to fix the other bugs that were introduced in that PR, but buck told me he would fix it after that point so I stopped working on it, which then turned into a refactor instead of just a bug fix and has been in limbo.

[UE4SS]

Yes it probably does need to be tested for actual release. Though, is it really necessary, if bitonality plans on changing the release CI/python script between now and the next actual release anyway? So I'd say it only needs to be tested for now on experimental, which it has.

I don't like the idea of pulling a partially untested PR. Regardless, when @bitonality and/or @narknon (I assume they want to test it first) approve the changes, a merge can happen. Much better testing needs to happen after making changes to CI in the future.

And @narknon, it's not feasible to wait for you to review and approve every PR. You're too inactive, we'd get nothing done. We failed to test the PR properly, that's something that shouldn't have happened, regardless if you were there or not, because we should be capable of testing properly without you

[narknon]

I wasn't bringing that up to call you guys out again, my point is that the script was broken when following the approval process, I broke it worse but I was aware of that and was actively working on it prior to development on it being shifted to someone else. So bit's point about following PR/approval is missing context here and that's not the reason there was an issue