Validate and publish

[lloeki]

Why?

FUP to #34 (chained PR)

What does this PR do?

Publish (conditionally) to rubygems.org; but before publishing, ensure that all variants of the gem:

• can be installed properly on every target platform
• can be used to compile C against the shared library on every target platform

How to test the change?

• CI should be green
• the publish step cannot be tested until an actual release happens

Additional Notes:

This is intentionally additive: the previous workflow is kept to allow a release "the old way" in short order until the "new way" is fully validated.

Once validated, a subsequent PR will remove the "old way" CI process and (optionally?) the fetch+extract Rake tasks.

JIRA:

[ivoanjo]

There's quite a bit of duplication here -- can we maybe push this inside Rake?

[lloeki]

I'm not sure we can have much in rake beyond this block:

          ruby -e '
            require "libdatadog"
            puts "version:    #{Libdatadog::VERSION}"
            puts "platform:   #{RUBY_PLATFORM}"
            puts "binaries:   #{Libdatadog.available_binaries}"
            puts "pkgconfig:  #{Libdatadog.pkgconfig_folder}"
            puts "receiver:   #{Libdatadog.path_to_crashtracking_receiver_binary}"
            raise "pkgconfig_folder is nil" unless Libdatadog.pkgconfig_folder
            raise "crashtracking receiver not found" unless Libdatadog.path_to_crashtracking_receiver_binary
          '

Is that what you mean?

[ivoanjo]

Why not everything else? E.g. installing the gem, running the smoke test etc -- at least to me rake is a lot easier to debug and test locally, and CI then could be a single "bundle exec rake do_the_testing"

[lloeki]

Why not everything else?

      - name: Start container
        run: docker run --rm --detach --name validate --volume "${PWD}:${PWD}" -w "${PWD}" ${{ matrix.platform.image }} sleep 86400

- uses: ruby/setup-ruby@7372622e62b60b3cb750dcd2b9e32c247ffec26a # v1.302.0

Well, we can't really install via rake without ruby? Also the execution model differs between macos and linux.

      - name: Download gem
        uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1

Not relevant to do locally? Or do you want to be able to download an artifact using a simple rake command, say, from an Actions URL? I could do add that as nice-to-have rake github:artifact:fetch[${id}] tooling to save clicking around, but I feel it's out of scope for this PR. Also I think we'd keep using the github artifact action in the workflow.

Alternatively there's rake libdatadog:build to build for the current platform.

      - name: Install gem
        run: gem install pkg/*.gem

This needs an artifact gem from above, otherwise there's standard rake install already.

      - name: Validate gem
        run: |
          ruby -e '
            require "libdatadog"
            puts "version:    #{Libdatadog::VERSION}"
            puts "platform:   #{RUBY_PLATFORM}"
            puts "binaries:   #{Libdatadog.available_binaries}"
            puts "pkgconfig:  #{Libdatadog.pkgconfig_folder}"
            puts "receiver:   #{Libdatadog.path_to_crashtracking_receiver_binary}"
            raise "pkgconfig_folder is nil" unless Libdatadog.pkgconfig_folder
            raise "crashtracking receiver not found" unless Libdatadog.path_to_crashtracking_receiver_binary
          '

Probably the most complex and useful to factor out; but then it would need the code checked out, which it currently doesn't have (see item just below)

Locally, even though it needs an artifact gem from above, it could be useful.

      - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
        with:
          sparse-checkout: spec/smoke

Locally, the repository is checked out already. But most importantly this check and all previous tests are done without ever cloning the repository: they essentially check that everything works fine outside of a development or development-like environment (gemfile around, etc...); that's why it does a sparse checkout in cone mode (subfolder).

      - name: Smoke test (compile and run C program against libdatadog)
        run: ruby spec/smoke/run.rb

The role of this test is to be run outside of the repo: checking that the installed gem behaves as expected in an isolated environment. Sure it can work in the repo too, and could be useful, but making it a rake task or a spec starts making it more dependent on a specific environment (Gemfile, task files, rspec/minitest/whatever, etc...). Admittedly the run.rb script could be a rake task that can be evaluated in both a dev context and this freestanding environment.

I aimed for simple here though. I'm OK to make some of these additions to have more useful tooling in subsequent PRs, but I think it is out of scope for this PR.

[ivoanjo]

I'm still kinda concerned that we have many things copy pasted many times and it's easy to change linux and forget to update macOS to match.

Doesn't need to be rake, but at least to me anything that's more than 1 line of trivial bash/ruby should go on a file and then we run that file, and then all variants can use that file/we can debug it trivially, rather than having it embedded in yaml multiple times.

[ivoanjo]

This is kinda weird/unexpected to me. If we want to make a release with the previous process, can we make it from a branch?

I'm slightly concerned that we're adding a lot of complexity to avoid using a branch 👀

[lloeki]

You mean that some code exists in both tasks/gem.rake and Rakefile?

My process here is to make sure the existing workflows don't break; especially publish.yml which can only be tested by publishing.

As I described in the PR:

This is intentionally additive: the previous workflow is kept to allow a release "the old way" in short order until the "new way" is fully validated.

Once validated, a subsequent PR will remove the "old way" CI process and (optionally?) the fetch+extract Rake tasks.

The idea behind the duplication is that some bits are copied over and changed as needed for the new gem building steps, but making them shared is both useless churn since they should go away once validated and more risky than "no change at all".

I get the feeling about the branch, but then the branch would get out of sync if/when a libdatadog update lands, plus while the worfklow_dispatch of the publish workflow allows for branch selection, it is unknown whether releasing from a branch actually works in code within the workflow, so I just thought it best to not change anything and be sure it's 100% identical, code or environment.

Once merged, on the next release we can do this process:

• attempt release with the new way
  • if succeeded:
    • open PR to remove the old way
  • if failed:
    • immediately proceed with release the old way
    • fix the new way without self-inflicted artificial pressure

It's a noop noop noop maneuver.

[ivoanjo]

The thing is, this repo exists for only one thing, release libdatadog. The kind of techniques you're describing are mostly for systems that need to keep running, when you're replacing the wheels on a car driving down the highway.

But for this repo, we can simplify! It only needs to do one thing, and only needs to do it when we want it to do it!

If this PR is not good, it's trivial to revert the whole PR, and we lose nothing. I really don't think it's worth going fancier -- let's just build in this PR what should be on master, and when it's ready, we merge it and test it, and if it's wrong we undo and try again. No need to get complicated.

[ivoanjo]

Gave it a pass, looks reasonable!

[ivoanjo]

Minor: Since packaging doesn't actually need the binaries it packages, we could run this all in one step and save on a bit of complexity (e.g. we don't need to run on macOS to package macOS, and those runners are always in worse shape)

[ivoanjo]

I'm still kinda concerned that we have many things copy pasted many times and it's easy to change linux and forget to update macOS to match.

Doesn't need to be rake, but at least to me anything that's more than 1 line of trivial bash/ruby should go on a file and then we run that file, and then all variants can use that file/we can debug it trivially, rather than having it embedded in yaml multiple times.

[ivoanjo]

Suggestion: Let's comment out this part, and then run the whole workflow and validate the gems manually before we do a separate PR to enable-this part.

[ivoanjo]

Should this be deleted, now we have the other flow?

[ivoanjo]

The thing is, this repo exists for only one thing, release libdatadog. The kind of techniques you're describing are mostly for systems that need to keep running, when you're replacing the wheels on a car driving down the highway.

But for this repo, we can simplify! It only needs to do one thing, and only needs to do it when we want it to do it!

If this PR is not good, it's trivial to revert the whole PR, and we lose nothing. I really don't think it's worth going fancier -- let's just build in this PR what should be on master, and when it's ready, we merge it and test it, and if it's wrong we undo and try again. No need to get complicated.

[ivoanjo]

Minor: We can get rid of this if we get rid of reading RUBY_PLATFORM directly -- that doesn't seem like something we need currently?

[ivoanjo]

Minor: I know this wasn't like this before, but I wonder if we should move to an allow-list instead, e.g. only package what we need, rather than excluding what we don't need