R4R: Makefile OS compatibility update #2709
Conversation
greg-szabo
requested review from
cwgoes,
ebuchman and
rigelrozanski
as code owners
Codecov Report
@@ Coverage Diff @@
## develop #2709 +/- ##
===========================================
- Coverage 56.69% 56.67% -0.03%
===========================================
Files 156 156
Lines 9790 9790
===========================================
- Hits 5550 5548 -2
- Misses 3862 3864 +2
Partials 378 378 |
Makefile
Outdated
| @@ -20,23 +18,31 @@ ci: get_tools get_vendor_deps install test_cover test_lint test | |||
| ######################################## | |||
| ### Build/Install | |||
|
|
|||
| check-ledger: | |||
| # Add ledger support - Makefile variable changes are out of a target's scope. There is no point in putting this into a target. | |||
There was a problem hiding this comment.
Is this a TODO? Im confused?
There was a problem hiding this comment.
Nope, it's a comment. We can remove it. The point is that checking the ledger support is not a target, it's a Makefile variable set - which is not a scope of a target. Everything below the comment adds ledger support, when it's available for build.
There was a problem hiding this comment.
Ok, then I don't think we need the comment then 👍
|
Guys, we need to move faster with this. The above changes were only a warmup. The ones coming today require deep knowledge of |
|
The good news is that the latest changes are actively used by CircleCI in the setup_dependencies stage. (That it succeeded, is a good sign.) The code was tested on OSX and Windows and now CircleCI ran it on Linux, so (apart from BSD) it cross-compiles properly. Changing the tool version is as easy as going to the bottom of the |
|
@greg-szabo Is this R4R or do you want to make more changes? |
|
It's ready for review. The first one was a valid change on its own. The second one is dependent on the first one. |
cwgoes
changed the title
Makefile
Outdated
| @@ -20,23 +18,31 @@ ci: get_tools get_vendor_deps install test_cover test_lint test | |||
| ######################################## | |||
| ### Build/Install | |||
|
|
|||
| check-ledger: | |||
| # Add ledger support - Makefile variable changes are out of a target's scope. There is no point in putting this into a target. | |||
There was a problem hiding this comment.
Ok, then I don't think we need the comment then 👍
|
What's holding back this change? |
|
Nothing afaik (except fixing the pending log conflict). |
Targeted PR against develop branch
Linked to github-issue with discussion and accepted design OR link to spec that describes this work. Makefile stabilization and updates #2672
Wrote tests - Ran tests on Windows and OSX
[] Updated relevant documentation (
docs/) (No feature change.)Added entries in
PENDING.mdwith issue #rereviewed
Files changedin the github PR explorerRequired for Voyager binary signing on Windows.
For Admin Use: