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

Default network for different compiled bins is still defaulting to Esme #5326

Closed
brianp opened this issue Apr 14, 2023 · 1 comment
Closed
Assignees

Comments

@brianp
Copy link
Contributor

brianp commented Apr 14, 2023

Problem

With the introduction of network based feature gating I attempted to apply a change to set different default networks for the different bin compliations.

This change did not have the desired effect and the NextNet binary still defaults to Esme. This is very confusing because a user would be immediately met with an error message informing them Esme is not a valid network choice for the NextNet binary.

The appropriate default was not used because the tari_common crate is not utilizing a build.rs, and in turn not utilizing the tari-feature gating configuration.

Solution

Introduce the tari-feature build settings to tari common. This will also introduce a cyclic dependency so it's more likely that we should move tari-features directly into tari-common.

@brianp brianp converted this from a draft issue Apr 14, 2023
@brianp brianp self-assigned this Apr 14, 2023
@brianp
Copy link
Contributor Author

brianp commented Apr 14, 2023

A little more thinking on this and moving tari-features into common doesn't work because we're saying common needs to be tari-feature aware.

I think what we need to do instead, is free tari-features of any outside dependency. It's only dep is tari-common, and it only uses the Network. I think we could probably address this as the problem.

@brianp brianp moved this from In Progress to In Review in Tari Esme Testnet Apr 17, 2023
SWvheerden pushed a commit that referenced this issue Apr 17, 2023
Description
---
Features is now a frequently used build dep in most our crates. Common
actually also needs to be feature aware during build time. This means
tari-features should have little to no dependencies, and especially none
from our own crates.

Motivation and Context
---
Related to issue #5326 
Related to nextnet hotfix #5327 

How Has This Been Tested?
---
Manually

What process can a PR reviewer use to test or verify this change?
---
Run cargo build with the desired network type (nextnet):
`TARI_NETWORK=nextnet cargo build --bin tari_base_node`

Run the bin directly without using cargo. It's important not to use
cargo during the testing as the bin will likely rebuild when using `run`
and change the previous `TARI_NETWORK` compilation settings:
`./target/tari_base_node` 

See that the default network is NextNet.

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify
SWvheerden pushed a commit that referenced this issue Apr 18, 2023
Description
---
Features is now a frequently used build dep in most our crates. Common
actually also needs to be feature aware during build time. This means
tari-features should have little to no dependencies, and especially none
from our own crates.

Motivation and Context
---
Related to #5326 
Development fix: #5333 

How Has This Been Tested?
---
Manually

What process can a PR reviewer use to test or verify this change?
---
Run cargo build with the desired network type (nextnet):
`TARI_NETWORK=nextnet cargo build --bin tari_base_node`

Run the bin directly without using cargo. It's important not to use
cargo during the testing as the bin will likely rebuild when using `run`
and change the previous `TARI_NETWORK` compilation settings:
`./target/tari_base_node` 

See that the default network is NextNet.

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify
@github-project-automation github-project-automation bot moved this from In Review to Done in Tari Esme Testnet Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

2 participants