makefile: propagate apidiff-compare opts from callers

[dnaeon]

Description

The api-compatibility workflow sets custom options for apidiff as part of the Compare-States and Check-States steps here.

• opentelemetry-collector/.github/workflows/api-compatibility.yml

  Line 58 in 6cd9c4e

  COMPARE_OPTS: -d "../${{ github.base_ref }}/internal/data/apidiff"

• opentelemetry-collector/.github/workflows/api-compatibility.yml

  Line 67 in 6cd9c4e

  COMPARE_OPTS: -d "../${{ github.base_ref }}/internal/data/apidiff" -c

These opts are picked up by the Makefile and collected in the APICOMPARE_OPTS var here.

• opentelemetry-collector/Makefile

  Lines 338 to 343 in 6cd9c4e

  	# If we are running in CI, change input directory
  	ifeq ($(CI), true)
  	APICOMPARE_OPTS=$(COMPARE_OPTS)
  	else
  	APICOMPARE_OPTS=-d "./internal/data/apidiff"
  	endif

However, these options are not propagated to apidiff when invoking the apidiff-compare target.

• opentelemetry-collector/Makefile

  Lines 345 to 348 in 6cd9c4e

  	# Compare API state snapshots
  	.PHONY: apidiff-compare
  	apidiff-compare:
  	@$(foreach pkg,$(ALL_PKGS),$(call exec-command,./internal/buildscripts/compare-apidiff.sh -p $(pkg)))

This PR propagates the APICOMPARE_OPTS to the apidiff-compare target, so that any configured options (e.g. by workflows) are respected.

Link to tracking issue

None

Testing

Documentation

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.60%. Comparing base (ee0bf52) to head (4d9c04d).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #14004   +/-   ##
=======================================
  Coverage   91.60%   91.60%           
=======================================
  Files         655      655           
  Lines       42782    42782           
=======================================
  Hits        39192    39192           
  Misses       2766     2766           
  Partials      824      824           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dmathieu]

I'm slightly surprised this worked and hasn't been changed since 2021 (#3464).

[dnaeon]

I'm slightly surprised this worked and hasn't been changed since 2021 (#3464).

Yeah, that's a long time :)

[dmathieu]

If we have been without this for so long, do we even need the opts?

[dnaeon]

If we have been without this for so long, do we even need the opts?

Maybe, maybe not :)

On one side things are working as expected today, however I think some changes might be needed.

• The workflow is misleading as it is today, because readers may assume these opts are actually in use, while they are not.
• The fact that the opts are not used at all, it actually makes Compare-States and Check-States steps identical (the Check-States step uses extra option -c for check-only, but this one is discarded).

Also, the compare-apidiff.sh and gen-apidiff.sh scripts default to internal/data/apidiff, which is populated by the Generate-States here, which might be the reason for "it just works", I'll need to double check these tomorrow once again :)

opentelemetry-collector/.github/workflows/api-compatibility.yml

Lines 49 to 52 in 6cd9c4e

	- name: Generate-States
	run: |
	cd $BASE_REF
	make apidiff-build

... and the workflow steps are supposed to execute against the branch from the PR.

Please hold this one for now, until things get more clarity :)

WDYT?

[dnaeon]

Hey @dmathieu ,

Had some time this morning to check things again and here's what I have so far.

I'm slightly surprised this worked and hasn't been changed since 2021 (#3464).

After going again through the gen-apidiff.sh, compare-apidiff.sh scripts, and the Inform Incompatible PRs workflow I believe this never actually worked correctly since the start.

Here's why.

1. gen-apidiff.sh creates state files for each package in internal/data/apidiff by default.

opentelemetry-collector/.github/workflows/api-compatibility.yml

Lines 49 to 52 in a96eab1

	- name: Generate-States
	run: |
	cd $BASE_REF
	make apidiff-build

opentelemetry-collector/internal/buildscripts/gen-apidiff.sh

Lines 17 to 21 in a96eab1

	dry_run=false
	package=""
	output_dir="./internal/data/apidiff"
	repo_toplevel="$( git rev-parse --show-toplevel )"
	tools_mod_file="${repo_toplevel}/internal/tools/go.mod"

2. Since the COMPARE_OPTS are never propagated from the workflow to the Makefile target, when the apidiff-compare target is called it uses its defaults as well.

opentelemetry-collector/.github/workflows/api-compatibility.yml

Lines 55 to 70 in a96eab1

	- name: Compare-States
	env:
	CI: true
	COMPARE_OPTS: -d "../${{ github.base_ref }}/internal/data/apidiff"
	run: |
	cd $HEAD_REF
	make apidiff-compare
	# Fail GitHub Action if there are incompatible changes
	- name: Check-States
	env:
	CI: true
	COMPARE_OPTS: -d "../${{ github.base_ref }}/internal/data/apidiff" -c
	run: |
	cd $HEAD_REF
	make apidiff-compare

opentelemetry-collector/internal/buildscripts/compare-apidiff.sh

Lines 17 to 21 in a96eab1

	package=""
	input_dir="./internal/data/apidiff"
	check_only=false
	repo_toplevel="$( git rev-parse --show-toplevel )"
	tools_mod_file="${repo_toplevel}/internal/tools/go.mod"

3. At this point in Compare-States step both the input_dir and output_dir for gen-apidiff.sh and compare-apidiff.sh are the same values (internal/data/apidiff), but the are in different parent directories, so no check is actually performed, because of this test here.

opentelemetry-collector/internal/buildscripts/compare-apidiff.sh

Line 47 in a96eab1

if [ -e "$input_dir"/"$package"/apidiff.state ]; then

Here's an example run of the workflow in my fork with debugging enabled, which shows that the step succeeds silently, without actually comparing anything.

https://github.com/dnaeon/opentelemetry-collector/actions/runs/18489346746/job/52679115017

Run cd $HEAD_REF
+ '[' -e ../main/internal/data/apidiff/go.opentelemetry.io/collector/internal/statusutil/apidiff.state ']'
+ '[' -e ../main/internal/data/apidiff/go.opentelemetry.io/collector/internal/testutil/apidiff.state ']'

In my fork I've also created one example PR, which introduces a breaking change in one of the internal/ packages, just to show that it is correctly detected and reported.

• internal/testutil: introduce a breaking API change in internal package dnaeon/opentelemetry-collector#6

And here's the workflow run for it.

https://github.com/dnaeon/opentelemetry-collector/actions/runs/18490457959/job/52682606364?pr=6

make: Entering directory '/home/runner/work/opentelemetry-collector/opentelemetry-collector/main'
make: Leaving directory '/home/runner/work/opentelemetry-collector/opentelemetry-collector/main'
make: Entering directory '/home/runner/work/opentelemetry-collector/opentelemetry-collector/test/apidiff-change'
Changes found in go.opentelemetry.io/collector/internal/testutil:
Incompatible changes:
- GetAvailableLocalAddress: changed from func(testing.TB) string to func(testing.TB) (string, error)
make: Leaving directory '/home/runner/work/opentelemetry-collector/opentelemetry-collector/test/apidiff-change'
make: Entering directory '/home/runner/work/opentelemetry-collector/opentelemetry-collector/test/apidiff-change'
Incompatible Changes Found.
Check the logs in the GitHub Action log group: 'Check-API-Compatibility'.
make: *** [Makefile:348: apidiff-compare] Error 1
make: Leaving directory '/home/runner/work/opentelemetry-collector/opentelemetry-collector/test/apidiff-change'
Error: Process completed with exit code 2.

I've also updated this PR with the changes to the workflow.

A couple of follow questions, which are not related to this PR, but might be implemented in future ones:

• Should we include the internal packages to the list of packages being checked for API compat?
• Should the rest of the modules (e.g. receiver, exporter, etc.) be checked for API compat as well?

Thanks!

[dnaeon]

PR rebased.

[dnaeon]

Hey, @dmathieu and @songy23 , can you please let me know if maintainers are interested in this PR, so that I know whether I should keep rebasing it or abandon it instead?

Thanks!