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

cpuminer: Refactor code to its own package. #2276

Merged
merged 2 commits into from
Jul 23, 2020

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Jul 19, 2020

This requires #2275.

This does the minimum work necessary to refactor the CPU miner code into its own internal package. The idea is that separating this code into its own package will improve its testability and ultimately be useful to other parts of the codebase such as the various tests which currently effectively have their own stripped-down versions of this code.

The API will certainly need some additional cleanup and changes to make it more usable outside of the specific circumstances it was originally designed to support (namely the generate RPC), however it is better to do that in future commits in order to keep the changeset as small as possible during this refactor.

Overview of the major changes:

  • Create the new package
  • Move internal/mining/cpuminer.go -> internal/mining/cpuminer/cpuminer.go
  • Update mining logging to use the new cpuminer package logger
  • Rename CPUMinerConfig to Config (so it's now cpuminer.Config)
  • Rename NewCPUMiner to New (so it's now cpuminer.New)
  • Add exported BestSnapshot method to mining.BlkTmplGenerator
  • Add exported TxSource method to mining.BlkTmplGenerator
  • Update all references to the cpuminer to use the package
  • Add a skeleton README.md
  • Add a skeleton doc.go

Copy link
Member

@dnldd dnldd 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 but needs a rebase.

@davecgh
Copy link
Member Author

davecgh commented Jul 19, 2020

Rebased. Will probably need a couple more as the other go in.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

I suppose there should also be a doc.go or a package comment somewhere. i.e. Package cpuminer provides facilities for solving blocks (mining) using the CPU. as a comment above package cpuminer somewhere.

@davecgh
Copy link
Member Author

davecgh commented Jul 21, 2020

@JoeGruffins Good call. Added a skeleton doc.go.

This does the minimum work necessary to refactor the CPU miner code into
its own internal package.  The idea is that separating this code into
its own package will improve its testability and ultimately be useful to
other parts of the codebase such as the various tests which currently
effectively have their own stripped-down versions of this code.

The API will certainly need some additional cleanup and changes to make
it more usable outside of the specific circumstances it was originally
designed to support (namely the generate RPC), however it is better to
do that in future commits in order to keep the changeset as small as
possible during this refactor.

Overview of the major changes:

- Create the new package
- Move internal/mining/cpuminer.go -> internal/mining/cpuminer/cpuminer.go
- Update mining logging to use the new cpuminer package logger
- Rename CPUMinerConfig to Config (so it's now cpuminer.Config)
- Rename NewCPUMiner to New (so it's now cpuminer.New)
- Add exported BestSnapshot method to mining.BlkTmplGenerator
- Add exported TxSource method to mining.BlkTmplGenerator
- Update all references to the cpuminer to use the package
- Add a skeleton README.md
- Add a skeleton doc.go
@davecgh davecgh merged commit 65da0cb into decred:master Jul 23, 2020
@davecgh davecgh deleted the cpuminer_package branch July 23, 2020 01:33
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.

5 participants