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

Fix 32-bit Windows releases so they are the correct architecture #1475

Merged
merged 16 commits into from
Jul 29, 2024

Commits on Jul 27, 2024

  1. Use bash for everything on all platforms in release workflow

    The way environment variables are being set and accessed assumes
    a Bourne-style (POSIX-like) shell, but PowerShell was being used on
    Windows for some steps where this was being assumed.
    
    It's possible to set bash only for some steps, but that is more
    complex to understand than using it for everything.
    EliahKagan committed Jul 27, 2024
    Configuration menu
    Copy the full SHA
    edd1c43 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    3e779d9 View commit details
    Browse the repository at this point in the history
  3. Remove confusing EXE_NAME environment variable

    This removes the `EXE_NAME` variable in the release workflow,
    replacing all uses of it with an literal `ein`, which was always
    its value.
    
    The rationale is that this variable was confusing in two ways:
    
    - The release workflow builds and publishes two executables, for
      both the ein and gix commands. The executable for the ein
      command was specified through this environment variable, while
      the executable for the gix command was already given literally.
      Besides improving consistency, removing the environment variable
      makes clear what is actually being specified.
    
    - The name of the executable that provides the ein command is `ein`
      on most operating systems but `ein.exe` on Windows. But the
      `EXE_NAME` variable always held the value `ein` on all systems,
      with a `.exe` suffix concatenated to it on Windows. This was
      unintuitive because `EXE_NAME` didn't always name an executable.
    EliahKagan committed Jul 27, 2024
    Configuration menu
    Copy the full SHA
    d7e9269 View commit details
    Browse the repository at this point in the history
  4. Copy from target-specific build directories even on Windows

    Changing the shell to bash *may* have been sufficient to cause
    these directories to be used by successfully allowing environment
    variables to be set and accessed using POSIX shell syntax. If so,
    then this change together with that one *may* be enough either to
    make it so that the 32-bit release archives really get 32-bit
    executables on Windows or to reveal further reasons they do not.
    EliahKagan committed Jul 27, 2024
    Configuration menu
    Copy the full SHA
    564bdc9 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    418d71f View commit details
    Browse the repository at this point in the history

Commits on Jul 29, 2024

  1. Minor changes from ripgrep workflow + simplify + clarify quoting

    The release.yml workflow adapts from the corresponding workflow in
    the ripgrep project, but drawing from multiple revisions of it and
    also including substantial gitoxide-specific changes.
    
    ripgrep's licensing permits this even with respect to any material
    that is original to that project, since one of the licenses ripgrep
    is offered under is the Unlicense, which allows arbitrary use and
    adaptation without requiring even attribution.
    
    This commit makes these changes related to that relationship:
    
    - Note the relationship, linking to the ripgrep workflow.
    
    - Bring in the new ripgrep comment explaining the workflow, which,
      as there, is shorter and just above the `create-release` job, not
      at the top. This makes a slight change, so as not to refer to a
      version tag scheme that differs and that we do not enforce.
    
    - Renames both *_VERSION variables to just `VERSION`, as they now
      are in the ripgrep workflow. These variables have conceptually
      the same meaning, always take the same value, and do not clash
      because each job in the workflow has exactly one of them.
    
    - Bring in the simplification of using `github.ref_name` to get the
      tag name to use as the version string.
    
    There remain other significant changes to that workflow that can
    and should be applied or adapted into this one, but which are not
    included in this commit.
    
    This commit also makes some other changes for simplification and
    stylistic clarity:
    
    - Change the trigger tag pattern to the glob-like `v*`, which seems
      to be what the GHA docs say will make it work (though this may
      have been tried unsuccessfully before).
    
    - Don't specify the fetch depth, because `actions-checkout`
      defaults to a depth of 1 already.
    
    - Remove ad-hoc echos that inadvertently performed empty parameter
      expansion because they used a variable that had just been written
      in `$GITHUB_ENV`. Writes to `$GITHUB_ENV` only affect the
      environment of subsequent steps, not of subsequent commands run
      as part of the same script step. These could have been shown in a
      different way, or by expanding them straightforwardly but in a
      subsequent script step (like the existing "Show command used for
      Cargo" step), but the removed echos did not seem very important
      and the information they were showing is mostly available by
      detailed log inspection.
    
    - Simplify some commands whose complexity was just to support those
      ineffective echos.
    
    - Use the bash `$(< path)` syntax where applicable. This is a more
      efficient alternative to `$(cat path)`. It's a bash-ism, but bash
      is now being used to run all steps on all platforms.
    
    - Where both (a) quoting was clearly unnecessary because of the way
      YAML parses strings and (b) quoting was being used in some places
      and not analogous other places, so the style was inconsistent,
      this removes the quoting. (This does not remove quoting in other
      situations, not even when one but not the other condition holds.)
    
    - When quoting inline, this always uses the strongest form, of
      those that are readily available and easily readable. This
      applies both to the formation of YAML strings, which now prefer
      single quotes to double quotes, and to quotation marks that
      appear inside YAML strings that are executed by the shell. Now
      double quotes are only used when both (a) quoting for a shell
      rather than the YAML parser and (b) deliberately causing the
      shell to perform its own `$...` expansions.
    
      This is because double-quoted `${{ }}` interpolation can falsely
      appear to be a shell expansion, when really all `${{ }}`
      interpolation is done after YAML parsing but before the shell
      receives the code that it runs in a script step.
    
    - Double-quote `$GITHUB_ENV` (rather than not quoting it), so all
      shell parameter expansion is double quoted except where expansion
      that quoting would suppress is wanted (currently, nowhere).
    
    - When shell quoting is optional because the item being `${{ }}`
      interpolated is guaranteed to be parsed by the shell as a single
      argument, and where we *rely* on that effect, this quotes the
      argument. This is done more for consistency, in view of how it
      was already mostly being done, than robustness. It is still not
      truly robust because, although we never set such values,  a `'`
      character, if present in the output of interpolation, would be
      (like all output of `${{ }}` interpolation) present literally in
      the script that the shell receives, and the shell would interpret
      it as a closing quotation mark.
    
    - Adjust spacing for consistency with the existing style.
    EliahKagan committed Jul 29, 2024
    Configuration menu
    Copy the full SHA
    286e388 View commit details
    Browse the repository at this point in the history
  2. Only use cross on Linux

    This adds the `if:` key from the ripgrep workflow to the
    "Use Cross" step, so that we only use `cross` instead of `cargo`
    when both of the following hold:
    
    (a) We are on a Linux runner.
    (b) It is possible that we are cross-compiling.
    
    Of those conditions, (a) is more important than (b), since `cross`
    can be used even when not cross-compiling. However, on Windows, it
    fails to create the environment, because there is no Docker image
    available for it, and also it is a Windows environment so Windows
    containers would have to be used, which are not available. This
    would also not currently be able to work on Windows hosted GHA
    runners, because they don't support nested virtualization.
    
    Although using `cross` on Windows in this workflow had never worked
    properly, there were two different incorrect behaviors:
    
    - It was previously not actually being used at all, because
      PowerShell rather than bash was being used as the shell, and the
      attempt to use syntax like `>> $GITHUB_ENV` in PowerShell did not
      succeed at setting the `CARGO` environment variable (nor others).
    
      Although this is *conceptually* related to the bug where the
      32-bit (i686) Windows release archives actually contained 64-bit
      binaries (GitoxideLabs#1472), it does not seem to have been the direct cause,
      since cross-compiling with `cargo` should have worked. But the
      related failure to set the `$TARGET_FLAGS` and `$TARGET_DIR`
      environment variables was an important factor.
      This was one factor responsible for the bug where the 32-bit
    
    - With bash used for all script steps on all operating systems, it
      failed early with an error from `docker` about not finding an
      image on ghcr.io corresponding to the target.
    
    For some further details, see:
    GitoxideLabs#1472 (comment)
    EliahKagan committed Jul 29, 2024
    Configuration menu
    Copy the full SHA
    a4b5cb9 View commit details
    Browse the repository at this point in the history
  3. Set $TARGET_FLAGS and $TARGET_DIR even without cross

    This adds that step from the revised ripgrep workflow, moving the
    commands into it. The new step is the same as in ripgrep except:
    
    - Without `shell: bash`, because we set that for the whole job
      and do so on all operating systems.
    
    - With a different quoting convention.
    
    The related change of taking the executables from the specific
    target directory has already been made. This is part of what is
    needed to make that actually work.
    EliahKagan committed Jul 29, 2024
    Configuration menu
    Copy the full SHA
    f6a71f0 View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    cf379f6 View commit details
    Browse the repository at this point in the history
  5. Don't check matrix.target for cross + do related prep

    All our jobs have matrix.target set, so that check isn't needed.
    The intent was to avoid using `cross` when not cross-compiling, but
    treating an empty value to mean native compilation wouldn't have
    had any effect.
    
    This also does some preparation for related forthcoming changes,
    moving the job-level `env` below `strategy` (so it remains easy to
    understand even once it gains matrix-influenced values) and fixing
    up some wording to reflect that we don't currently build for any
    bigendian architectures such as s390x.
    EliahKagan committed Jul 29, 2024
    Configuration menu
    Copy the full SHA
    cb10aae View commit details
    Browse the repository at this point in the history
  6. Set $TARGET_FLAGS and $TARGET_DIR from matrix

    Since they are produced via `${{ }}` interpolation using values
    from the matrix.
    
    Matrix values cannot be formed from job-level `env`, but job-level
    `env` can be formed form matrix values, as this does.
    EliahKagan committed Jul 29, 2024
    Configuration menu
    Copy the full SHA
    8f43a92 View commit details
    Browse the repository at this point in the history
  7. Configuration menu
    Copy the full SHA
    fbafff3 View commit details
    Browse the repository at this point in the history
  8. Don't set CARGO=cross too early

    Because `cargo` can also use this environment variable.
    EliahKagan committed Jul 29, 2024
    Configuration menu
    Copy the full SHA
    79f9ec0 View commit details
    Browse the repository at this point in the history
  9. Use shell expansions for env vars where clearer

    This uses parameter expansion, and sometimes also brace expansion,
    to make some commands that use environment variables shorter and
    easier to read.
    
    I did not apply brace expansion in the strip command run in docker,
    since the shell running that may be sh rather than bash.
    EliahKagan committed Jul 29, 2024
    Configuration menu
    Copy the full SHA
    d6769a6 View commit details
    Browse the repository at this point in the history
  10. Don't use -- argument to strip

    Unlike many utilities, the `strip` utility is not required by
    POSIX to follow the XBD 12.2 Utility Syntax Guidelines, so need not
    support `--`. On macOS, and possibly some other systems, it does
    not, and `--` causes an error. Fortunately, it's okay to just not
    pass it, because the value of `$TARGET_DIR` is known not to start
    with `-`.
    EliahKagan committed Jul 29, 2024
    Configuration menu
    Copy the full SHA
    a647598 View commit details
    Browse the repository at this point in the history
  11. Go back to always using cross on Linux

    Because technically even the "non-cross" compilation is cross
    compilation, as it is targeting `x86_64-unknown-linux-musl`.
    
    When `cargo` rather than `cross` is used in that build, `ring`
    fails to compile C code due to the `musl-gcc` command not being
    present. Very likely other dependencies would also fail without
    more tools installed on the system, and future features we wish to
    support in binary releases would run into this in further ways.
    
    An alternative approach here could be installing `musl-gcc` in a
    prior step, much as we could also install an ARM gcc suite and
    probably eliminate `cross` for linux-arm. For now, though, I'm
    putting this back to using `cross` for all Linux builds.
    EliahKagan committed Jul 29, 2024
    Configuration menu
    Copy the full SHA
    3c3a831 View commit details
    Browse the repository at this point in the history