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

ci: add ci to prevent future pr's breaking sim #124

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KolbyML
Copy link

@KolbyML KolbyML commented Jan 9, 2024

No description provided.

@KolbyML KolbyML marked this pull request as ready for review January 9, 2024 15:17
@KolbyML KolbyML marked this pull request as draft January 9, 2024 15:18
@KolbyML KolbyML force-pushed the add-test branch 3 times, most recently from ee967c6 to 5cc8ced Compare January 9, 2024 18:44
@KolbyML KolbyML marked this pull request as ready for review January 9, 2024 18:53
@KolbyML
Copy link
Author

KolbyML commented Jan 9, 2024

@marten-seemann ok the PR is ready for review. I tested it on master and it failed as expected. I also tested it with the Fix Lars summited and the test passed as expected.

The test is very minimal in scope, but it prevents the event that happened yesterday so it does its job.

Cheers!

@KolbyML KolbyML requested a review from marten-seemann January 9, 2024 18:56
@KolbyML KolbyML force-pushed the add-test branch 3 times, most recently from d8fad24 to 7b57861 Compare January 9, 2024 19:27
Copy link
Collaborator

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Thank you @KolbyML for working on this!

Building the simulator takes a long time (~1h) on CI. I'd prefer if we could make this an additional step in the build-and-push workflow (i.e. first build, then test, then push), instead of adding a new workflow, for 2 reasons:

  1. obviously, build time
  2. it's safest to test exactly the image that we're about to push to DockerHub

@KolbyML
Copy link
Author

KolbyML commented Jan 10, 2024

@marten-seemann it only takes
image
7-8 minutes. I am not sure what you are referring to when you say (~1h) on CI

Also yeah for sure that is no issue!

@marten-seemann
Copy link
Collaborator

Huh, this is weird. What's going on here? Compiling ns3 from scratch takes more than an hour, see https://github.com/quic-interop/quic-network-simulator/actions/runs/7465935034/job/20328138279?pr=124

@KolbyML
Copy link
Author

KolbyML commented Jan 10, 2024

Maybe that is being caused by qemu? I did docker build -f sim/Dockerfile . and it was always taking less then 7 minutes on ci I was testing it all morning

@KolbyML
Copy link
Author

KolbyML commented Jan 10, 2024

In that case I don't think we should combine the CI

I am going to rebase the pr to show it becomes green with Lars fix

@KolbyML
Copy link
Author

KolbyML commented Jan 10, 2024

Here is a picture taken on broken master for future reference
image

@KolbyML
Copy link
Author

KolbyML commented Jan 10, 2024

@marten-seemann you have to approve the CI to run. But anyways it will be successful and run in 7-8 minutes which is not 90 minutes so I don't think we should merge the 2 CI's together. QEMU do be slow though

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