Roblox / foreman

Toolchain manager for Roblox projects
MIT License
171 stars 26 forks source link

Prefer 64-bit artifacts for windows/x86_64. #48

Closed Anaminus closed 2 years ago

Anaminus commented 2 years ago

If arch is 32-bit, then only 32-bit artifacts are detected. If arch is non-32-bit, then 32-bit artifacts are still included, but with reduced priority.

NOTE: I have no idea what I'm doing.

Anaminus commented 2 years ago

This change alone won't be enough to make foreman prioritize assets which their name contains win32 over windows.

The "windows" keyword is used as a fallback in case an asset with "win32" doesn't exist. Because "windows" doesn't indicate the architecture, it has less priority. This is how the code already works before this PR.

The logic for asset selection on windows right now can be represented with the following pseudocode:

for each asset
    if asset name contains "win32" then
        select asset
    else if asset name contains "win64" then
        select asset
    else if asset name contains "windows" then
        select asset
    else
        continue
    end
end

With this PR, different conditions are used depending on the architecture. The conditions are ordered to prioritize keywords that are the best fit for the architecture:

if target_arch == "x86" then
    for each asset
        if asset name contains "win32" then
            select asset
        else if asset name contains "windows" then
            select asset
        else
            continue
        end
    end
else
    for each asset
        if asset name contains "win64" then
            select asset
        else if asset name contains "windows" then
            select asset
        else if asset name contains "win32" then
            select asset
        else
            continue
        end
    end
end

In particular, 64-bit will try to select "win32" only after it has tried all other options, and 32-bit avoids selecting "win64" entirely.

github-actions[bot] commented 2 years ago

CLA Signature Action:

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you read and sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just by adding a comment to this pull request with this exact sentence:

I have read the CLA Document and I hereby sign the CLA

By commenting with the above message you are agreeing to the terms of the CLA. Your account will be recorded as agreeing to our CLA so you don't need to sign it again for future contributions to our company's repositories.

0 out of 1 committers have signed the CLA. :x: @Anaminus

oltrep commented 2 years ago

Apply the following list of assets to the algorithm you just shared 🙂

[ "tool-windows.zip" , "tool-win32.zip" ]

It will select the first one, but we would actually want the second one.

Anaminus commented 2 years ago

That is a problem already present with foreman's selection algorithm that is not introduced by this PR, and is beyond the scope of this PR.

Regardless, it appears I am required to sign a CLA in order for this PR to be merged, which I will not be doing. Someone else is free to make this change if they desire.