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

pkg/private: cleanup common, util, xtest packages #4311

Merged
merged 12 commits into from
Feb 9, 2023

Conversation

matzf
Copy link
Contributor

@matzf matzf commented Jan 30, 2023

The pkg/private/common, util and xtest packages have rather fuzzy scope, and have accumulated a bit of cruft and unused or outdated functionality. Clean this up a bit:

  • pkg/private/common:
  • pkg/private/util:
    • remove unused helper functionality
    • remove Checksum: only used to compute reference value in slayers test cases. Use a simpler, non-optimized implementation for this. Closes checksum: re-evaluate library #4262.
    • move RunsInDocker to private/env
    • move ASList to tools/integration
  • pkg/private/xtest:
    • remove unused helpers
    • remove unused Callback and MockCallback
    • replace FailOnErr with require.NoError
    • replace AssertErrorsIs with assert.ErrorIs

There are still more things to clean up in pkg/private, in future PRs, in particular:

  • common.ErrMsg should be integrated in serrors
  • common.IFIDType should be removed or renamed and moved somewhere more appropriate
  • Merge the remainder of util and common
  • Clean up LinkType and RevInfo from pkg/private/ctrl

This change is Reviewable

@matzf matzf requested review from oncilla and a team as code owners January 30, 2023 10:44
Copy link
Contributor

@marcfrei marcfrei left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 59 of 59 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 59 of 59 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf and @oncilla)


tools/integration/integration.go line 1 at r1 (raw file):

// Copyright 2018 Anapaya Systemduring

revert that

Suggestion:

Systems

Copy link
Contributor Author

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 58 of 59 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker, @marcfrei, and @oncilla)


tools/integration/integration.go line 1 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

revert that

Yes, oops! Done.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @oncilla)

@matzf matzf force-pushed the cleanup-common-util branch from 5abc504 to c6fe8ef Compare February 3, 2023 15:29
Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 58 of 59 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @matzf)


pkg/private/util/checksum.go line 25 at r3 (raw file):

// Calculate RFC1071 checksum of supplied data chunks. If a chunk has
// an odd length, it is padded with a 0 during checksum computation.
func Checksum(srcs ...[]byte) uint16 {

Just as an FYI, this implementation was ~2 times as fast as the slayers implementation the last time I benchmarked.
But given it is not used, we don't need to keep it around.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @matzf)

@matzf matzf merged commit 756c8b4 into scionproto:master Feb 9, 2023
@matzf matzf deleted the cleanup-common-util branch February 9, 2023 15:16
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.

checksum: re-evaluate library
4 participants