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

Build improvements #680

Closed
wants to merge 12 commits into from
Closed

Build improvements #680

wants to merge 12 commits into from

Conversation

gmega
Copy link
Member

@gmega gmega commented Jan 18, 2024

Edit: I've done some more extensive reasoning on this here.

Currently, our CI file which runs tests allow one to specify a compiler version. I argue this is not desirable, as neither the Dockerfile nor local builds will use that version. Indeed, the version that gets picked up in those other cases is whatever version nimbus-build-system uses as its default, which is another thing I'd argue is undesirable. 🙂

What would be desirable instead would be if: i) the versions in CI, Dockerfile, and local builds (without overrides) would always agree; and ii) we could decouple those from nimbus-build-system defaults, as those may or may not match what we want at any given time.

This PR does two changes that try to remedy this:

  1. it removes the ability to specify a compiler version from the CI;
  2. since the compiler is already built as part of Codex anyway, it bakes the version information into the build system.

This will ensure that the compiler version is consistent across tests and the docker image, and also simplifies the CI as a side effect. In addition, a developer can always specify a different version by setting a different value for NIM_COMMIT locally.

Finally, this PR wipes clean the misleading information contained in the .nimble file, which lists neither the correct dependencies, nor their correct version. It also purges all unused/unmaintained lockfiles, including atlas', as those do not reflect the correct versions either (see this spreadsheet for the discrepancies wrt the nimble lockfile, similar issues plague the atlas lockfile).

@gmega gmega marked this pull request as draft January 18, 2024 20:29
@gmega gmega marked this pull request as ready for review January 18, 2024 20:44
@dryajov
Copy link
Contributor

dryajov commented Jan 18, 2024

I argue this is not desirable, as neither the Dockerfile nor local builds will use that version.

We should not remove the ability to specify the compiler version, we should in fact build against all the versions that we support - currently it's 1.6.18, but there will be more once things stabilize a little bit.

We should bump the project to the latest supported version (1.6.18), by bumping nimbus-build-system dep in the vendors directory, in fact the latest master points to 1.6.18. Also, the makefile allows you to select the version you want to build against with the NIM_COMMIT variable - checkout https://github.com/status-im/nimbus-build-system?tab=readme-ov-file#nim_commit.

In short, the CI should always build against all the supported compilers and we usually do maintain compatibility with at least 2-3 past versions.

@gmega
Copy link
Member Author

gmega commented Jan 18, 2024

The CI currently does not build against all supported versions. This would be a larger change, and would require us to figure how to make this scale without a build taking hours (which I believe was the reason why we never made the matrix larger than one single version in the original CI script).

That said, this PR does allow you to change the version by specifying the NIM_COMMIT before sourcing env.sh, so definitely won't prevent that.

@@ -9,9 +9,6 @@ inputs:
cpu:
description: "CPU to build for"
default: "amd64"
nim_version:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not remove this

Copy link
Member Author

@gmega gmega Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I argue in my doc that this is not really achieving much ATM. While I agree that building against a compiler matrix is useful, what this currently does is pin the a CI to a single version, which is the wrong one. :-)

@gmega
Copy link
Member Author

gmega commented Jan 19, 2024

@dryajov (and whoever else lays eyes on this) I've done some more extensive reasoning for this here. In short, we can support a compiler matrix when needed.

gmega added a commit that referenced this pull request Jan 19, 2024
gmega added a commit that referenced this pull request Jan 19, 2024
@gmega gmega marked this pull request as draft January 19, 2024 20:31
@gmega gmega mentioned this pull request Jan 22, 2024
@gmega gmega marked this pull request as ready for review January 22, 2024 14:26

- name: Build Nim and Codex dependencies
shell: ${{ inputs.shell }} {0}
run: |
make -j${ncpu} CI_CACHE=NimBinaries ARCH_OVERRIDE=${PLATFORM} QUICK_AND_DIRTY_COMPILER=1 update
./env.sh make -j${ncpu} CI_CACHE=NimBinaries ARCH_OVERRIDE=${PLATFORM} QUICK_AND_DIRTY_COMPILER=1 update
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Makefile already imports the env

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a different env, though.

gmega added a commit that referenced this pull request Jan 23, 2024
@veaceslavdoina
Copy link
Contributor

In short, the CI should always build against all the supported compilers and we usually do maintain compatibility with at least 2-3 past versions.

One thing keep in ming about possible limitation with GitHub Actions - Usage limits.

Screenshot 2024-01-25 at 18 00 42

Just because running a lot of concurrent jobs, we may block other jobs in same repository or across entire org.

And some results from the test we performed

details
jobs:
  build:
    strategy:
      matrix:
        nim_version: [v1.6.14, v1.6.16, v1.6.18]
        os: [linux, macos, windows]
        arch: [amd64]
        tests: [unittest, contract, integration, all]
        include:
          - os: linux
            builder: ubuntu-latest
            shell: bash --noprofile --norc -e -o pipefail
          - os: macos
            builder: macos-latest
            shell: bash --noprofile --norc -e -o pipefail
          - os: windows
            builder: windows-latest
            shell: msys2
        exclude:
          - os: linux
            tests: unittest
          - os: linux
            tests: contract
          - os: linux
            tests: integration
          - os: macos
            tests: unittest
          - os: macos
            tests: contract
          - os: macos
            tests: integration
          - os: windows
            tests: all
Screenshot 2024-01-25 at 14 17 25 Screenshot 2024-01-25 at 14 17 47

@veaceslavdoina
Copy link
Contributor

Context

For now we have 3 separate build flows

  1. Local builds
  2. CI builds
  3. Docker builds

As we already discussed we have at lest 2 ways to pass a custom Nim version for builds

  1. Existing one - use a NIM_COMMIT variable
  2. PR proposed way, or a variation of that, using a version specified in the nim-codex repository

It should also be noted that a default version currently is defined as a submodule from in the nimbus-build-system repository.

Existing one - use a NIM_COMMIT variable

Current situation

  1. We already can pass a custom version to local builds
  2. We already can pass a custom version to CI builds
  3. Docker builds need to be adjusted to support that

Pros

  1. No changes required to the local and CI builds
  2. A well known native way

Cons

  1. We should take care about all builds flows and locations where Nim version should be updated
  2. Default version is defined out of the base repository

PR proposed way

Current situation

  1. Local, CI and Docker builds will use same version from an existing env.sh file

Pros

  1. Does not break a native way and we can pass NIM_COMMIT to override Nim version
  2. A single source for all existing build flows

Cons

  1. Necessity to run ./env make

Other examples

No one is using a way to override Nim version? How do they handle that?

  1. https://github.com/status-im/status-desktop/blob/master/env.sh
  2. https://github.com/status-im/nimbus-eth2/blob/stable/env.sh
  3. https://github.com/waku-org/nwaku/blob/master/env.sh

Reference

Questions

  1. Is there a native way to pass Nim version for all our builds flows?
  2. In the proposed PR, can we load env.sh from Makefile to not run ./env.sh make every time?

@gmega
Copy link
Member Author

gmega commented Jan 30, 2024

Projects seem to organize things differently, and tolerate different levels of inconsistencies. Nimbus uses a compiler matrix in CI where they subscribe to release streams (e.g. version-1-6), and not specific versions.

They then have a set of Docker images which awkwardly appear to be used to build binary tarballs, and then some hollow images to actually distribute the binaries, in what looks like an emulation of a multistage build. These appear to scoop whatever version of the Nim compiler is defined in the nimbus-build-system by default, and does not allow for configuration.

Waku has a much simpler system with no compiler matrix. They just scoop the defaults for nimbus-build-system everywhere, and as far as I can tell cannot experiment with custom compiler versions in CI or Docker images at all.

So answering your first question, @veaceslavdoina, there doesn't seem to be a consistent way of overriding Nim versions or keeping those things in sync. This is a bit less of a problem for Nimbus as nimbus-build-system is... well, their build system. But still, creating a branch to test a new compiler version does not appear to be feasible unless you create a branch of nimbus-build-system itself, set it to a different default, and then use that branch as a submodule into your own branch (quite cumbersome).

As for your second question: I suppose we can do away with invoking env.sh if we modify the top-level Makefile.

@gmega gmega self-assigned this Feb 29, 2024
@gmega
Copy link
Member Author

gmega commented Jun 12, 2024

I have a better idea on how to do this now, so will close this.

@gmega gmega closed this Jun 12, 2024
@gmega gmega deleted the feat/build-improvements branch June 12, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants