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

software_interrupt: Add additional testing #260

Merged
merged 1 commit into from
Jun 4, 2021
Merged

software_interrupt: Add additional testing #260

merged 1 commit into from
Jun 4, 2021

Conversation

josephlr
Copy link
Contributor

@josephlr josephlr commented Jun 4, 2021

So I just realized we already had some of the changes in #259 merged as part of #227.

This PR adds the rest of the testing code. CC: @Freax13

Signed-off-by: Joe Richey [email protected]

@phil-opp
Copy link
Member

phil-opp commented Jun 4, 2021

So we accidentally resetted the next branch and removed the changes from #259 from it? Perhaps we should make next a protected branch to avoid issues like this in the future. Edit: Seems like the branch protection was already turned on, so I'm not sure how this happened.

@josephlr
Copy link
Contributor Author

josephlr commented Jun 4, 2021

So we accidentally resetted the next branch and removed the changes from #259 from it? Perhaps we should make next a protected branch to avoid issues like this in the future. Edit: Seems like the branch protection was already turned on, so I'm not sure how this happened.

I just added the setting that prevents force pushing (after I realized my mistake in moving the next branch). I think I didn't have the permissions setup correctly last time.

EDIT: Looked though my Git history and this was definitely my bad. I force pushed the branch while trying to fix things with #259 (I had forgotten that next was a "real" branch), now that I've fixed the permissions it shouldn't be possible to make that mistake again.

@josephlr
Copy link
Contributor Author

josephlr commented Jun 4, 2021

@phil-opp do you think we should have the "Require pull request reviews before merging" and "Include Administrators" settings checked for the next and master branches? This is the setup we have for getrandom

@phil-opp
Copy link
Member

phil-opp commented Jun 4, 2021

@josephlr No worries, I just tried to understand what was going on.

do you think we should have the "Require pull request reviews before merging" and "Include Administrators" settings checked for the next and master branches? This is the setup we have for getrandom

Sure, we can try that if you have time for regular reviews. Not directly related, but I would also like to automate the release creation process through GitHub actions (e.g. use the GitHub Api to detect whether the current crate version is present on crates.io and do a cargo publish if not). This way, releasing a new version would be as easy as changing the version number in the Cargo.toml through a PR.

@josephlr josephlr enabled auto-merge (squash) June 4, 2021 08:20
@josephlr josephlr disabled auto-merge June 4, 2021 08:20
@josephlr
Copy link
Contributor Author

josephlr commented Jun 4, 2021

Sure, we can try that if you have time for regular reviews.

Given that we're starting to use this crate more at work, I think I'll be able to commit more time here. Also, can anyone in the @rust-osdev org approve changes for this repo?

I just enabled the settings and confirmed that I can no longer merge this PR without your approval.

Not directly related, but I would also like to automate the release creation process through GitHub actions (e.g. use the GitHub Api to detect whether the current crate version is present on crates.io and do a cargo publish if not). This way, releasing a new version would be as easy as changing the version number in the Cargo.toml through a PR.

I think that would be an optimal way to go about things, anything that makes the release process less painful is fine by me.

@josephlr josephlr enabled auto-merge (squash) June 4, 2021 08:23
@phil-opp
Copy link
Member

phil-opp commented Jun 4, 2021

Given that we're starting to use this crate more at work, I think I'll be able to commit more time here.

Great to hear that! I would love to hear more about how you're using this crate. Anything you can talk about?

I just enabled the settings and confirmed that I can no longer merge this PR without your approval.

👍

I think that would be an optimal way to go about things, anything that makes the release process less painful is fine by me.

Ok, cool. I try to look into this soon.

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@josephlr josephlr merged commit 33e8c3b into next Jun 4, 2021
@josephlr josephlr deleted the intr branch June 4, 2021 08:31
This was referenced Jun 4, 2021
josephlr added a commit to josephlr/x86_64 that referenced this pull request Jul 18, 2021
Followup to rust-osdev#259. Code previously merged as part of rust-osdev#227.

Signed-off-by: Joe Richey <[email protected]>
Freax13 pushed a commit to Freax13/x86_64 that referenced this pull request Oct 17, 2021
Followup to rust-osdev#259. Code previously merged as part of rust-osdev#227.

Signed-off-by: Joe Richey <[email protected]>
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 this pull request may close these issues.

2 participants