Add migration guide for the nnsdk/nnware split - #87
Pistonight wants to merge 1 commit into
Conversation
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 2 files and all commit messages, and made 9 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on Pistonight).
MIGRATE.md line 12 at r1 (raw file):
| other repos compatibility update | Before 2027 | | Downstream projects update | Before 2027 | | Archive this repo | Before 2027 |
following the naming scheme of migrating related libraries
Most projects will encounter issues due to the other libraries making a forced switch eventually, but the removal of backwards-compatible solutions is still in the future, and project authors might want to get notified about the removal before that happens. Additionally, some games/projects might only use the nnheaders repo and none of the others, so this one should also loudly complain about "you need to take an action!" before being archived.
Suggestion:
| Archive this repo | Before 2027 |
In the process of archival, CMakeLists will throw an error if this repo is continued to be used, unless `USE_OLD_NNHEADERS_REPO` is set. This is to ensure that downstream projects are explicitly requested to migrate to the new setup, while still giving them an escape route to continue using this one.MIGRATE.md line 60 at r1 (raw file):
The current headers don't follow consistent nameing convention and placement. With the migration, we also want to take the opportunity to fix that.
Suggestion:
The current headers don't follow consistent naming convention and placement. With the migration,
we also want to take the opportunity to fix that.MIGRATE.md line 110 at r1 (raw file):
Review the diff afterwards, since the regex also matches names in comments and strings, and drop the `nn/nn_Result.h` include from files that don't use `nn::Result`. - Use the same aliases from another library, for example `<prim/seadTypes.h>` from `sead`
Suggestion:
basis/seadTypes.hMIGRATE.md line 140 at r1 (raw file):
# commit the new submodule git commit -m "add new nnsdk and nnware"
Suggestion:
# commit the new submodule
git add .
git commit -m "add new nnsdk and nnware"MIGRATE.md line 154 at r1 (raw file):
# you only need to link what you need, the example shows all 3 libraries target_link_libraries(my_project PRIVATE nnSdk nn_gfx nvn)
What are the commands to add nnware here?
Code quote:
Then, change the `CMakeLists.txt` for your project and their dependencies:
```cmake
# old:
add_subdirectory(lib/NintendoSDK)
target_link_libraries(my_project PRIVATE NintendoSDK)
# new:
add_subdirectory(lib/nnsdk)
# you only need to link what you need, the example shows all 3 libraries
target_link_libraries(my_project PRIVATE nnSdk nn_gfx nvn)___
*[`MIGRATE.md` line 158 at r1](https://reviewable.io/reviews/open-ead/nnheaders/87#-P2Chi4k4gvWqkTlS6UY:-P2Chi4k4gvWqkTlS6UZ:b45h) ([raw file](https://github.com/open-ead/nnheaders/blob/e9b705ef311015a3c148fe0779829e481fb7c1f3/MIGRATE.md#L158)):*
> ```Markdown
> Re-run CMake and clean-build your project to make sure everything builds fine, then, remove the old
> repo (assuming it's at `lib/NintendoSDK`):
> ```
> ```
_Suggestion:_
```Markdown
```bash
MIGRATE.md line 169 at r1 (raw file):
# commit the removal git add .gitsubmodule git commit -m "removed old NintendoSDK"
it should be .gitmodules, but just doing . should be good to add everything
Suggestion:
# commit the removal
git add .
git commit -m "removed old NintendoSDK"MIGRATE.md line 237 at r1 (raw file):
Each PR will retain the current commits by the PR author at the time of porting, a port commit will be authored on top, followed by a merge commit into the `main` branch.
Suggestion:
All current PRs, including the WIP ones, will be ported and merged to the new repos.
The new repos are already in good shape, so please direct future contributions there,
and do not open new PRs on this repo.
Each ported PR will retain the current commits by the PR author at the time of porting,
a port commit will be authored on top, followed by a merge commit into the `main` branch.README.md line 3 at r1 (raw file):
# nnheaders **We are undergoing migration to split the SDK and NintendoWare, please do not create new PRs here during the migration. See [Migration Guide](./MIGRATE.md) for what this means for your project that depends on nnheaders**
What is that letter height in reviewable - this looks like yOUr on my screen 🙃
Code quote:
your
Pistonight
left a comment
There was a problem hiding this comment.
@Pistonight made 6 comments.
Reviewable status: 1 of 2 files reviewed, 8 unresolved discussions (waiting on MonsterDruide1).
MIGRATE.md line 12 at r1 (raw file):
Previously, MonsterDruide1 wrote…
following the naming scheme of migrating related libraries
Most projects will encounter issues due to the other libraries making a forced switch eventually, but the removal of backwards-compatible solutions is still in the future, and project authors might want to get notified about the removal before that happens. Additionally, some games/projects might only use the nnheaders repo and none of the others, so this one should also loudly complain about "you need to take an action!" before being archived.
I will make the timeline more explicit about what downstream projects are expected to see in each phase
MIGRATE.md line 154 at r1 (raw file):
Previously, MonsterDruide1 wrote…
What are the commands to add
nnwarehere?
Done.
MIGRATE.md line 169 at r1 (raw file):
Previously, MonsterDruide1 wrote…
it should be
.gitmodules, but just doing.should be good to add everything
Done.
MIGRATE.md line 60 at r1 (raw file):
The current headers don't follow consistent nameing convention and placement. With the migration, we also want to take the opportunity to fix that.
Done.
MIGRATE.md line 158 at r1 (raw file):
Re-run CMake and clean-build your project to make sure everything builds fine, then, remove the old repo (assuming it's at `lib/NintendoSDK`):
Done.
MIGRATE.md line 237 at r1 (raw file):
Each PR will retain the current commits by the PR author at the time of porting, a port commit will be authored on top, followed by a merge commit into the `main` branch.
Done.
Pistonight
left a comment
There was a problem hiding this comment.
@Pistonight made 2 comments.
Reviewable status: 1 of 2 files reviewed, 8 unresolved discussions (waiting on MonsterDruide1).
MIGRATE.md line 110 at r1 (raw file):
Review the diff afterwards, since the regex also matches names in comments and strings, and drop the `nn/nn_Result.h` include from files that don't use `nn::Result`. - Use the same aliases from another library, for example `<prim/seadTypes.h>` from `sead`
Done.
MIGRATE.md line 140 at r1 (raw file):
# commit the new submodule git commit -m "add new nnsdk and nnware"
Done.
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 1 file and all commit messages, and resolved 8 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Pistonight).
leoetlino
left a comment
There was a problem hiding this comment.
@leoetlino reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on Pistonight).
README.md line 3 at r1 (raw file):
Previously, MonsterDruide1 wrote…
What is that letter height in reviewable - this looks like
yOUron my screen 🙃
looks fine on my side? (I'm on Reviewable)
|
@leoetlino @MonsterDruide1 please review the final draft with the information for STATIC vs OBJECT target added |
MonsterDruide1
left a comment
There was a problem hiding this comment.
@MonsterDruide1 reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Pistonight).
MIGRATE.md line 188 at r5 (raw file):
> above, you will find some functions are now missing from the final binary, this is because > when linking a static library (`.a`), only objects in the library that contain a symbol > that is currently undefined are linked, whereas `.o` files are always linked.
the lower part is weirdly phrased - instead of going from the side of "currently undefined", I'd write something like
[...] only objects in the library that are referenced in the main project
Suggestion:
> above, you will find some functions are now missing from the final binary. This is because
> when linking a static library (`.a`), only objects in the library that are referenced
> in the main program are linked, whereas `.o` files are always linked.MIGRATE.md line 191 at r5 (raw file):
> > To fix this, we need to tell the linker to always link everything in the archive, > with the `--whole-archive` ld flag. In CMake, specify the `WHOLE_ARCHIVE` feature
Suggestion:
using
To understand the context please read from this discord message
This change is