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

Renaming / reorganization #5

Closed
3 of 4 tasks
ian-h-chamberlain opened this issue Aug 10, 2023 · 9 comments · Fixed by #20
Closed
3 of 4 tasks

Renaming / reorganization #5

ian-h-chamberlain opened this issue Aug 10, 2023 · 9 comments · Fixed by #20

Comments

@ian-h-chamberlain
Copy link
Member

ian-h-chamberlain commented Aug 10, 2023

There are a couple of things I want to consider here. Each of these could probably be a separate issue, but I think to some extent they'll all happen together at once so just tracking here for now:

  • Moving .github/actions/* to either the top level or their own repository. I think this repo makes sense, particularly since the Citra test runner requires some level of coordination with the GDB test runner, but it's not necessarily the recommended location. This would also make it easier to version the two actions (setup and run) separately, otherwise we might need to use tags like setup@setup-v1 or something.

    Also, if actions are moved to the top level, maybe the test-runner crate should also be moved down into its own directory?

  • Naming; this crate doesn't ever need to be published to https://crates.io, but if if is published it will need a sufficiently unambiguous name. test-runner-3ds is kinda long, maybe just like test3ds or something more creative?

  • Moving to the rust3ds org. This seems obvious and is just a matter of making sure the CI and everything still works once moved.

  • Finish documenting the actions in the README at least, to make it more obvious how they're meant to work.

@ian-h-chamberlain ian-h-chamberlain pinned this issue Aug 10, 2023
@ian-h-chamberlain
Copy link
Member Author

ian-h-chamberlain commented Sep 30, 2023

This is done and I decided to just call it test-runner. I'll leave this issue open for now, in case we think of a better name but I think being under the rust3ds organization makes it fairly clear what it's meant for... If we ever publish to crates.io then a different crate name will be necessary of course.

Edit: I realized the setup action also not make as much sense being in a repo named test-runner... Maybe we'd want something a little more generic like devtools or even just actions, then you'd have something like

    steps:
      # ...
      - name: Setup Rust3DS toolchain
        uses: rust3ds/actions/setup@v1

      - name: Build and run tests
        uses: rust3ds/actions/run-tests@v1

It might feel a bit odd to have the test-runner crate in this repo then, though, especially if it's made a little more general purpose. I think the versioning should probably still match with the actions, since there might be implementation details that have to match up between the GDB/Citra container and the GdbRunner.

test-runner = { git = "https://github.com/rust3ds/actions", tag = "v1" } 
# or if we rename and publish to crates.io ?
test-runner-3ds = "1.0.0" 

For now we can leave as-is but something to think about especially if other project start using this framework at all.

@Meziu
Copy link
Member

Meziu commented Oct 1, 2023

I wouldn't exclude this crate from the "release list". One day people may want to use it to implement automated testing in their own applications (which now that I think about it, I've never seen a 3DS app with automated tests like ours before).

Obviously the actions/test-runner have to be matching, but we should think of the possibility of publishing this crate to crates.io and see people using it easily from there.

That also means it needs a cool name 😉.

@ian-h-chamberlain
Copy link
Member Author

That also means it needs a cool name 😉.

Not sure if this is a "cool" name exactly, but how about ctrun or ctrunner? Googling them turns up https://developer.apple.com/documentation/coretext/ctrun-61n for ctrun and not much for ctrunner, but both look to be available on https://crates.io and sort of tie into the naming of ctru, Citra, etc.

I was thinking it might not need to exactly be named as test-related, since it might also eventually be used to run examples or other binaries, with a bit of tweaking.

So now I'm thinking maybe we split this into two repositories, so we can version the actions separately at least:

  • rust3ds/setup (or maybe setup-action): GitHub Action for setting up the toolchain, installing cargo-3ds etc.

  • rust3ds/??? (maybe test, test-action, or just ctrun[ner]?)

    • ctrun[ner] crate for #[test_runner] and doctest setup (Rust-specific, obviously).

    • GitHub Action for running binaries / tests (probably could be language-agnostic if we do it right, although needs gdbHioDevInit or equivalent for stdout/stderr to work).

      If this is agnostic enough we could also make it a separate repository too, but it could be awkward if there is a versioned contract between the crate and the action, so I think one repo makes more sense for now.

Thoughts?

@Meziu
Copy link
Member

Meziu commented Oct 16, 2023

Not sure if this is a "cool" name exactly, but how about ctrun or ctrunner?

CTR* names are a bit overused already in my opinion. There is the cedrus tree which comes to mind when thinking about Citra (cedrun? sounds very goofy), but in general I think it doesn’t need to tie in with the other repos’ naming structure too much.

I was thinking it might not need to exactly be named as test-related, since it might also eventually be used to run examples or other binaries, with a bit of tweaking.

That’s a very good point. Imo we can keep the repository as one for now, but we should also keep in mind the possibility of splitting it if one or both modules grow independent of each other.

@AzureMarker
Copy link
Member

AzureMarker commented Dec 24, 2023

It might be worth moving this to the ctru-rs repo, since they're interdependent. test-runner uses ctru-rs as a dependency, and ctru-rs uses test-runner as a dev dependency.

For example, this PR on ctru-rs (rust3ds/ctru-rs#86) causes test-runner to require a change (#15), but this test-runner change is required to make the ctru-rs PR checks pass.

Edit: The test-runner PR isn't needed anymore after recent changes in the ctru-rs PR, but the dependency is still there.

@ian-h-chamberlain
Copy link
Member Author

It might be worth moving this to the ctru-rs repo, since they're interdependent. test-runner uses ctru-rs as a dependency, and ctru-rs uses test-runner as a dev dependency.

Yeah, and this was one reason I originally thought it might be better to leave in the ctru-rs repo, but there is another factor, which is that this repo's run-tests Docker image also sort of depends on the gdb_runner implementation in https://github.com/rust3ds/test-runner/blob/main/test-runner/src/gdb.rs — so I guess it comes down to which API we're more comfortable with "stabilizing".

  • The GDB contract is pretty simple, I guess in principle it's just what's described in GDB docs and we rely on the __ctru_exit function to report exit codes properly. I guess that's basically it...?
  • The Rust API is a bit more finicky, as you encountered there is definitely some danger of subtly breaking compilation (everything described in https://doc.rust-lang.org/cargo/reference/semver.html basically), and the almost-circular dependency we have setup makes it more likely to be troublesome. Of course these kinds of breaking changes also have potential to break downstream users, but it's less likely since they can update ctru and test_runner at the same time.

After I've thought about it enough to write all that down, I think you might be right:

  • test-runner can probably move back to the ctru-rs repo (but as a separate crate instead of just a module like it was before, maybe we just call it ctru-runner or something)
  • This repo becomes a dedicated GitHub actions repo, since we already have two: setup and run-tests. I'm not sure if we're likely to add any more, but possibly as it relates to e.g. libc ABI checks or something

Does that sound like a reasonable plan? I think it eliminates the kinds of concerns @AzureMarker brought up in terms of the ctru -> test-runner -> ctru dependency, and probably also makes it easier to eventually publish them all to crates.io simultaneously, while hopefully giving other people the option to use our GitHub Actions as well.

@Meziu
Copy link
Member

Meziu commented Jan 7, 2024

I’ve had problems with rust-analyzer and even cargo due to the dependency cycle. I guess the crate should be moved to ctru-rs, but the actions may be useful to have here.

@Meziu
Copy link
Member

Meziu commented May 3, 2024

The issue is closed, but we should take the chance and rename the repo to avoid confusion.

@ian-h-chamberlain
Copy link
Member Author

Done. I think GitHub will keep forwarding references to the old name here, but we'll probably want to go through and update them for clarity at some point too.

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 a pull request may close this issue.

3 participants