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

Don't hash shared libraries for cache key #10841

Merged
merged 1 commit into from Aug 4, 2022

Conversation

ChrisBr
Copy link
Contributor

@ChrisBr ChrisBr commented Jul 28, 2022

Shared libraries often contain timestamps of when they were compiled and other non-stable data
hence would often produce different hashes on different machines. This happens often in our CI.

Related to #8633

cc @casperisfine


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@ChrisBr ChrisBr force-pushed the cbruckmayer/fix-cache branch 2 times, most recently from 0a4ad57 to 5120a4b Compare July 28, 2022 12:01
@ChrisBr
Copy link
Contributor Author

ChrisBr commented Jul 28, 2022

I'm not sure the failed tests are related 🤔

@ChrisBr ChrisBr marked this pull request as ready for review July 28, 2022 12:01
@ChrisBr ChrisBr force-pushed the cbruckmayer/fix-cache branch 2 times, most recently from ab49bc5 to 97fa45f Compare July 28, 2022 12:11
if path.end_with?(*DL_EXTENSIONS)
# shared libraries often contain timestamps of when
# they were compiled and other non-stable data
path

Choose a reason for hiding this comment

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

Hum, actually when we hash the .rb files, we don't care where they are located. So using the path as a "digest" mean that the BUNDLE_PATH now has influence, that may also be a problem for some cases. Maybe we should just extract a small name, e.g. Zlib.crc32(File.basename(path)).to_s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good point.

@ChrisBr ChrisBr force-pushed the cbruckmayer/fix-cache branch 2 times, most recently from c851d23 to 30eb8cd Compare July 29, 2022 10:41
@bbatsov bbatsov requested a review from jonas054 August 1, 2022 09:29
Copy link
Collaborator

@jonas054 jonas054 left a comment

Choose a reason for hiding this comment

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

Looks good to me, regarding the changed functionality. I had some minor comments.

lib/rubocop/result_cache.rb Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
lib/rubocop/result_cache.rb Outdated Show resolved Hide resolved
@ChrisBr
Copy link
Contributor Author

ChrisBr commented Aug 2, 2022

♻️ @jonas054 please have another look

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 3, 2022

Hmm, lots of tests are currently failing.

@ydah
Copy link
Member

ydah commented Aug 3, 2022

Looks like there are some RuboCop offense in lib/rubocop/result_cache.rb

Run bundle exec rake internal_investigation
RuboCop failed!
Running RuboCop...
Inspecting 1403 files
........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................W..................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

Offenses:

lib/rubocop/result_cache.rb:208:9: C: [Correctable] Layout/IndentationWidth: Use 2 (not -8) spaces for indentation.
        File.basename(path)
        ^^^^^^^^
lib/rubocop/result_cache.rb:209:7: C: [Correctable] Layout/ElseAlignment: Align else with if.
      else
      ^^^^
lib/rubocop/result_cache.rb:211:7: W: [Correctable] Layout/EndAlignment: end at 211, 6 is not aligned with if at 205, 16.
      end
      ^^^

1[40](https://github.com/rubocop/rubocop/runs/7642059901?check_suite_focus=true#step:8:41)3 files inspected, 3 offenses detected, 3 offenses autocorrectable
Error: Process completed with exit code 1.

@ChrisBr
Copy link
Contributor Author

ChrisBr commented Aug 3, 2022

Hmm, lots of tests are currently failing.

Fixed my Rubocop violation and rebased on latest master but seems there are still some broken in CI / RSpec 4 (pull_request). I don't think it's related to the changes in this PR though (if so I would appreciate some pointers how I can reproduce this locally, I did run bundle exec rake default which passed).

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Shared libraries often contain timestamps of when they were compiled and other non-stable data
hence would often produce different hashes on different machines
@koic koic merged commit 382761c into rubocop:master Aug 4, 2022
@koic
Copy link
Member

koic commented Aug 4, 2022

Thanks!

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

6 participants