-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversion to Pluggable Miners #22
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Will start writting helper jsons as soon as get some time |
Include default miner configuration files to be overridden Point to configurations files in miner directory
Adding Foreman name mapping and correcting cuda-devices ethminer argument
My first attempt ...
@LuKePicci |
@LuKePicci |
About the first point, I was thinking for a solution to this, maybe the easiest thing is to make this dynamic by grepping it from the output as well. About the second point, the reason to have miner executables named like they are created when compiling is that the copy command can simply consist in a list of files to be copied. If the copy command would need to rename the compiled binary output to something else, I will need to add code to perform such operation (easy) but also add another filed to manifests which specify which is the name of the output binary to be renamed. Can be done but is is just a complication we can avoid leaving miners executables with their names. For closed-source miner this issue, obviously, does not exist. Please, check in the manifest you updated for unescaped quotes ", they will break the json file. Just escape them with \ and it should be ok. You can check syntax with an online json validator, if you need it. |
Revert z-enemy binary name to default
Fixed json errors and renamed binary to default, |
Looks good, I'm now editing the initial post for this PR to include z_ewbf and z-enemy, if you add anything else please keep it updated (github should let you edit it). |
@LuKePicci |
Yes, definitely, but I would rather suggest to merge this branch directly or via another PR for that same branch, keeping this PR open for tracking further changes. I'm quite busy atm but if you think you can get in troubles by doing so I could also do it for you in a few days - hopefully. |
No rush, I am a bit busy a.t.m. too. |
This PR is intended to keep an open discussion about conversion of each currently supported miner to the PM format and allow quick testing and review of a work that is expected to take a lot of time.
The plan is to convert step-by-step one miner per time. As soon as we get evidence of a limitation in the surrounding PM framework which is preventing a correct conversion of a particular miner I will try working on it to proceed further. At the end, when we will have a clear idea about how each miner has been adapted we could discuss about cleaning up and refactoring the pm framework behind them taking the best from discussed issues.
I hereby invite any nvOC insider to try writing a PM adaptation of existing builtin miners and to submit PRs against this branch. It is an invaluable help since it is quite easy to do following existing examples (see contents of existing JSON manifests), but it is a long and boring work - once you get the idea it is kind of a repetition.
Note: this branch should not require any change to the main nvOC scripts so it is easy for you to test, just switch your nvOC_miners submodule to this branch and - if you give your PM experiment a custom name - change the miner name in 1bash accordingly. If a PM exists with the same name of a builtin one it takes precedence over the latter.
Here is the list of currently converted miners (working ✔ / untested ⚠ / missing ❌):