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

Nix hygiene #4511

Merged
merged 6 commits into from
Mar 27, 2025
Merged

Nix hygiene #4511

merged 6 commits into from
Mar 27, 2025

Conversation

lloeki
Copy link
Member

@lloeki lloeki commented Mar 18, 2025

What does this PR do?

  • Keep nix shell up to date
  • Fix incorrect direct pkgs reference instead of the desired pinned llvm
  • Add more ruby versions

Motivation:

Up to date nix shell

Change log entry

Nope

Additional Notes:

How to test the change?

@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.75%. Comparing base (18a0a6d) to head (18c76ce).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4511      +/-   ##
==========================================
- Coverage   97.76%   97.75%   -0.01%     
==========================================
  Files        1392     1392              
  Lines       84890    84890              
  Branches     4277     4277              
==========================================
- Hits        82995    82988       -7     
- Misses       1895     1902       +7     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@datadog-datadog-prod-us1
Copy link
Contributor

datadog-datadog-prod-us1 bot commented Mar 18, 2025

Datadog Report

Branch report: lloeki/update-nix
Commit report: 18c76ce
Test service: dd-trace-rb

✅ 0 Failed, 20782 Passed, 1370 Skipped, 3m 16.85s Total Time

@pr-commenter
Copy link

pr-commenter bot commented Mar 18, 2025

Benchmarks

Benchmark execution time: 2025-03-26 15:19:04

Comparing candidate commit 18c76ce in PR branch lloeki/update-nix with baseline commit 18a0a6d in branch master.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 30 metrics, 2 unstable metrics.

scenario:tracing - Propagation - Datadog

  • 🟥 throughput [-3505.940op/s; -3422.188op/s] or [-10.881%; -10.621%]

@lloeki lloeki marked this pull request as ready for review March 18, 2025 16:49
@lloeki lloeki requested a review from a team as a code owner March 18, 2025 16:49
@p-datadog
Copy link
Member

I am not seeing the nix failures that are currently attached to this PR in other PRs, therefore I assume they are somehow related to the changes made by this PR and I think those failures need to be investigated and repaired before merging.

@lloeki lloeki force-pushed the lloeki/update-nix branch from bb7976b to e047cd7 Compare March 26, 2025 10:22
@lloeki
Copy link
Member Author

lloeki commented Mar 26, 2025

The full error is:

An error occurred while loading ./spec/datadog/tracing/contrib_spec.rb. - Did you mean?
                    rspec ./spec/datadog/tracing/contrib/http_spec.rb
                    rspec ./spec/datadog/tracing/contrib/grpc_spec.rb
                    rspec ./spec/datadog/tracing/contrib/patcher_spec.rb

Failure/Error: root = Gem::Specification.find_by_name('datadog').gem_dir

Gem::MissingSpecVersionError:
  Could not find 'datadog' (>= 0) - did find: [datadog-2.12.2]
  Checked in 'GEM_PATH=/Users/runner/work/dd-trace-rb/dd-trace-rb/vendor/bundle/ruby/3.4.0' , execute `gem env` for more information
# ./spec/datadog/tracing/contrib_spec.rb:4:in 'block in <top (required)>'
# ./spec/datadog/tracing/contrib_spec.rb:3:in '<top (required)>'

Interestingly enough it's not that it doesn't see the local repo's datadog but that it sees a datadog-2.12.0 and considers it not matching with the datadog >= 0 constraint 🤔 .

This does return the spec:

Gem::Specification.specification_record.find_all_by_name("datadog")

@lloeki
Copy link
Member Author

lloeki commented Mar 26, 2025

So I got down the rabbit hole:

https://github.com/rubygems/rubygems/blob/557237b70c8f22e016742c046dac6531dbc95be1/lib/rubygems/dependency.rb#L282

In the failing case (this PR):

irb(main):001> Gem::Specification.specification_record.find_all_by_name("datadog").first.ignored?
=> true

In the success case (master)

irb(main):001> Gem::Specification.specification_record.find_all_by_name("datadog").first.ignored?
/nix/store/nwzyvr91mw3qhclgl3np1wxvy15gnxg1-ruby-3.3.5/lib/ruby/3.3.0/rubygems/specification.rb:2121:in `method_missing': undefined method `ignored?' for an instance of Gem::Specification (NoMethodError)

This traces us to this commit:

rubygems/rubygems@c80998a

Whose title is:

Fix specs with missing extensions getting activated

The PR being rubygems/rubygems#8104

Well extensions are missing because they have bot been built. Most notably on darwin some of them cannot:

$ bundle exec rake compile

+------------------------------------------------------------------------------+
| Could not compile the Datadog Continuous Profiler because                    |
| macOS is currently not supported by the Datadog Continuous Profiler.         |
|                                                                              |
| The Datadog Continuous Profiler will not be available,                       |
| but all other datadog features will work fine!                               |
|                                                                              |
| Get in touch with us if you're interested in profiling your app!             |
+------------------------------------------------------------------------------+

Ergo datadog is an ignored? spec.

Therefore the test as it is written fails "legitimately": it essentially means that the datadog gem is not in a clean state for testing.

But it is quite arguable that what's written there is what should be done. Indeed the only point of this access to the spec is to get to gem_dir; extensions don't matter, so Gem.loaded_specs is sufficient.

In any case, the issue is unrelated to Nix.

lloeki added a commit that referenced this pull request Mar 26, 2025
`rubygems` started enforcing extensions to be built via
`Gem::Specification#ignored?`.

This causes the `datadog` spec lookup via
`Gem::Specification.find_by_name` to fail.

Since in this case we don't care for the extension, replace by
`Gem.loaded_specs`, which is what matters there.

Note that `Gem::SpecificationRecord.find_all_by_name` is not subject to
the `ignored?` check, only `Specification.find_by_name` is (via
`Gem::Dependency#to_spec`), so we could use that; unfortunately it is
fairly recent and unavailable for older rubygems versions.

See:

- rubygems/rubygems@c80998a
- rubygems/rubygems#8104
- #4511 (comment)
@lloeki lloeki requested a review from a team as a code owner March 26, 2025 12:02
lloeki added a commit that referenced this pull request Mar 26, 2025
`rubygems` started enforcing extensions to be built via
`Gem::Specification#ignored?`.

This causes the `datadog` spec lookup via
`Gem::Specification.find_by_name` to fail.

Since in this case we don't care for the extension, replace by
`Gem.loaded_specs`, which is what matters there.

Note that `Gem::SpecificationRecord.find_all_by_name` is not subject to
the `ignored?` check, only `Specification.find_by_name` is (via
`Gem::Dependency#to_spec`), so we could use that; unfortunately it is
fairly recent and unavailable for older rubygems versions.

See:

- rubygems/rubygems@c80998a
- rubygems/rubygems#8104
- #4511 (comment)
@lloeki lloeki force-pushed the lloeki/update-nix branch from dfcdfc9 to 31b29ba Compare March 26, 2025 12:07
lloeki added 6 commits March 26, 2025 15:54
`rubygems` started enforcing extensions to be built via
`Gem::Specification#ignored?`.

This causes the `datadog` spec lookup via
`Gem::Specification.find_by_name` to fail.

Since in this case we don't care for the extension, replace by
`Gem.loaded_specs`, which is what matters there.

Note that `Gem::SpecificationRecord.find_all_by_name` is not subject to
the `ignored?` check, only `Specification.find_by_name` is (via
`Gem::Dependency#to_spec`), so we could use that; unfortunately it is
fairly recent and unavailable for older rubygems versions.

See:

- rubygems/rubygems@c80998a
- rubygems/rubygems#8104
- #4511 (comment)
@lloeki lloeki force-pushed the lloeki/update-nix branch from 31b29ba to 18c76ce Compare March 26, 2025 14:54
@lloeki lloeki requested a review from p-datadog March 26, 2025 14:55
@lloeki lloeki merged commit 1fa3344 into master Mar 27, 2025
255 checks passed
@lloeki lloeki deleted the lloeki/update-nix branch March 27, 2025 13:41
@github-actions github-actions bot added this to the 2.13.0 milestone Mar 27, 2025
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.

3 participants