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.

Sign up for GitHub

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

Jump to bottom

Andrew Q pipeline run #1509

Closed
wants to merge 8 commits into from
Closed

Andrew Q pipeline run #1509

wants to merge 8 commits into from
+71 −244

Conversation

coolkingcole
Copy link
Collaborator

testing pipeline for Andrew Quijano.

AndrewQuijano and others added 8 commits July 1, 2024 15:22
@AndrewQuijano
Delete travis and publish_deb
b89176b
@AndrewQuijano
Update install_ubuntu.sh to download libosi and capstonev5, remove de…
b231c93 
…ad variables from setup.py in pypanda, and optimize setup.sh to only use one panda container
@AndrewQuijano
Update documentation pointing out new debian packages and PyPanda on …
80c6124 
…Pip is now in sync with GitHub releases
@AndrewQuijano
Bump Actions versions to the latest, cache test container to DockerHu…
b1658c6 
…b instead of GHCR, enforce publish_deb can only proceed after parallel_tests, and finally, use Org secret instead of repo secret
@AndrewQuijano
Use latest version of LibOsi, add popd to solve build.sh issue, and a…
50db25c 
…llow parallel_tests to be run via workflow dispatch for external users
@AndrewQuijano
Also do NOT forget the ldconfig
5fbc6d1
Merge remote-tracking branch 'andrew-fork/dev' into andrew-pipeline-run
bd55fe8
add v to tag
4eef1a8
AndrewFasano
AndrewFasano reviewed Jul 11, 2024
View reviewed changes
panda/scripts/install_ubuntu.sh
Comment on lines -156 to -157
$SUDO python3 -m pip install -r requirements.txt
$SUDO python3 setup.py install
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is good practice to use sudo with pip install and it seems the installer test does pass just fine if we don't use sudo.
https://askubuntu.com/questions/802544/is-sudo-pip-install-still-a-broken-practice

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, this change can be undone and the commit can be squashed. I figure once LAVA is upgraded I'd also know for sure if using sudo to install pypanda is critical or not.

AndrewFasano
AndrewFasano reviewed Jul 11, 2024
View reviewed changes
.github/workflows/publish_docker.yml
Comment on lines +4 to +8
workflow_run:
workflows: ["Parallel Tests"]
types:
- completed

Copy link
Member

Choose a reason for hiding this comment

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

I believe this will break CI. parallel tests run on PRs. Publish docker runs on pushes to dev. With this change I believe publish_docker will never run.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I believe you are right, this change will be undone and squashed

@AndrewFasano
Copy link
Member

AndrewFasano commented Jul 11, 2024 •
edited
Loading

I'm fairly reluctant to merge changes to our CI as we can't really test CI in advance and it typically requires a series of PRs to fix the things each change breaks. There are a lot of different things going on here. I like some of them, but definitely don't think they should all be in a single PR. There are still lots of things mixed in/across commits in ways that make it hard to understand your changes.

Here's my best attempt to identify what's happening here and provide feedback. I'm very interested in merging the docs updates and action version increments. I'm worried the other changes will break things.

  • b89176b: deleting stale files - great
  • b231c93
    • Rename containers in debian/setup.sh - why? Seems harder to understand that there are 2 distinct containers now that they both have the same name
    • Update libcapstone to install v5 via git - good (I guess there's no v5 package)
    • Switch to installing libosi via a released package instead of git - great
    • Change install_ubuntu.sh to install pypanda as the local user (though without the --user flag?) - maybe best practice, but shouldn't be mixed in with these other changes
  • 80c6124: update docs - great
  • b1658c6
    • Update CI actions to new versions - this seems good and worth merging, even if we have to troubleshoot later
    • Break publish_docker here Not good
    • Rename containers/where they get pushed - I know we talked about this, does this resolve the issue where external collaborators can't have their PRs get tested (even after the PR is approved to run in CI)? If not, why risk changing this
    • Change build action for (unused?) panda_stable container - fine
  • 50db25c to update libosi version - looks good, should be squashed with changes in b231c93 (which could be split into multiple commits)
  • 5fbc6d1 to run ldconfig - looks good, should also be squashed with relevant changes in b231c93.
  • bd55fe8 merge commit / not sure if any of this matters / goes away if we rebase on main.
  • 4eef1a8 should be squashed with b231c93.

@AndrewQuijano
Copy link
Contributor

AndrewQuijano commented Jul 15, 2024 •
edited
Loading

So as for my change to setup.sh, currently for deployment we:

1- create a panda_installer image up to installer target
2- create a panda image up to panda target

As this script is only used for deployment, I propose just creating one panda image, build up to installer, extract the wheel file, and continue to build until panda target to package the Debian package. This should speed up deployment quite a bit as we are running make for panda only once instead of twice, at least that is what I assume the "CACHED" means on the screenshots provided.

image
image
image

Here is the full run log
https://productionresultssa7.blob.core.windows.net/actions-results/ccb307b3-5aa8-49f9-88f2-63447c0a8652/workflow-job-run-23cbbf46-0090-5639-7e11-ae079f190a1b/logs/job/job-logs.txt?rsct=text%2Fplain&se=2024-07-15T04%3A26%3A12Z&sig=E0aXzZEK6NmDb2r0dxEPnaIWOFOtOt7M4Q%2B0anbSMEw%3D&ske=2024-07-15T14%3A38%3A20Z&skoid=ca7593d4-ee42-46cd-af88-8b886a2f84eb&sks=b&skt=2024-07-15T02%3A38%3A20Z&sktid=398a6654-997b-47e9-b12b-9515b896b4de&skv=2023-11-03&sp=r&spr=https&sr=b&st=2024-07-15T04%3A16%3A07Z&sv=2023-11-03

About creating pandare/panda_test, the issue is that external users can't push to ghcr.io/pandare, so I propose we cache on DockerHub, since sharing a secret is easier than creating a shared account. Based on what I read, it seems the solution then is to call a Workflow dispatch button, so that then the external PR workflow can access the dockerhub secret and cache the image in DockerHub. It seems like the caching worked just fine. Unfortunately, it seems like there is no way to know if this will work until it is merged and when an external party submits a PR. But I'd say this meets both security needs as dispatch workflow can only occur manually, but would allow external users to use the cached dockerhub API token to cache a test panda container.

https://docs.github.com/en/actions/using-workflows/manually-running-a-workflow

P. S.
For libcapstone, I just followed what you did on the Dockerfile
f861fb3

@AndrewFasano AndrewFasano changed the title Andrew pipeline run Andrew Q pipeline run Jul 15, 2024
@AndrewQuijano
Copy link
Contributor

Everything should be addressed on this branch:
https://github.com/AndrewQuijano/panda/tree/cole-updates

@AndrewQuijano
Copy link
Contributor

And I raised the issue to get a Debian package for Capstone, seems like whenever v6 comes out, we will finally get a package

capstone-engine/capstone#2398

@AndrewQuijano AndrewQuijano mentioned this pull request Jul 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrewFasano AndrewFasano AndrewFasano left review comments

@AndrewQuijano AndrewQuijano AndrewQuijano left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@coolkingcole @AndrewFasano @AndrewQuijano