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

Modify project structure in preparation for Upstream #13

Conversation

george-cosma
Copy link

This pull request solves issue #12

This pull request contains the following changes:

  • The python folder and its files have been removed
  • The rust folder has been removed and its files have been moved to the root of the project
  • A simple workflow has been added to build and test any push to "rust-port" or pull request with the target branch "rust-port"

To test the workflow, I've created a dummy pull request which includes a failing test.
george-cosma#2

Questions

There are two issues I'd like to discuss regarding the workflow:

  1. What should happen to the old workflows? Would it be appropriate to delete them?
  2. Should the rust workflow be ran for any push/pull on any branch? Maybe all branches except main?

If my understanding of workflows is correct, github runs the workflows if the corresponding config files are found in the source branch of the pull request.

If this is true, then I think we could delete the python-specific workflows because a contributor to the rust version would pull from the "rust-port" branch, where the python workflows would not need to exist, and a contributor to the python version would pull from the "master" branch, where the rust workflow does not exist. Or so I think.

And in regards to my second question, I'm still uncertain of the advantages and disadvantages of testing all pull requests VS testing only the pull requests going towards the "rust-port" branch.

@alexandruradovici
Copy link

  1. As we do not merge this into master, we can delete the workflows that are python related. As soon as this is merged into master, the python version will be deprecated.
  2. You have specified which branch the workflow affects, my understanding is that the workflow will run only on this branch, it might not run on branches forked from this. I would suggest not to put a branch name, merge this and test if the workflow works on a branched forked and does not affect the master branch.

@alexandruradovici alexandruradovici linked an issue Apr 10, 2023 that may be closed by this pull request
3 tasks
@george-cosma george-cosma marked this pull request as ready for review April 10, 2023 11:19
@george-cosma
Copy link
Author

george-cosma commented Apr 10, 2023

I've removed all branch restrictions on that workflow and it works as it should:

  1. A fork of the rust version will be checked by the rust workflow on pull request (dummy file george-cosma/tockloader#3)
  2. A fork of the main version will be checked by the python workflow on pull request (dummy file main george-cosma/tockloader#4)

There is one issue: PR on rust-port trigger the tests twice: once for being a pull request, and one for being a push.
image

A possible fix would be to limit the push event to only the "rust-port" branch, but Pull Requests will be checked regardless of target.

@alexandruradovici
Copy link

This is how Tock does all the testing, so that should be fine.

@george-cosma
Copy link
Author

Alright, I'll leave it as-is then

.github/workflows/rust.yml Outdated Show resolved Hide resolved
tools/run_clippy.sh Outdated Show resolved Hide resolved
tools/run_fmt_check.sh Outdated Show resolved Hide resolved
@george-cosma
Copy link
Author

I've looked at how Tock does their CI pipeline, and I must admit I'm quite a fan.

They want to make sure all checks can be ran on the local version of the repository while keeping code repetition to a minimum. They do this by invoking rules in a Makefile.

In my latest commits, I've taken heavy inspiration from them. I've also made two more test forks to make sure they work properly.

Failing PR: george-cosma#5
Passing PR: george-cosma#6

Now, initially, I used a naming scheme similar to that of tock's Makefile, for the sake of consistency. Now, I'm not so certain, they might be a little verbose.

@alexandruradovici alexandruradovici merged commit 97bbb37 into UPB-CS-OpenSourceUpstream:rust-port Apr 13, 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

Successfully merging this pull request may close these issues.

Submit the rust branch to upstream
2 participants