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/Tests: use a container image to avoid installing klayout in each job #54

Closed
wants to merge 1 commit into from

Conversation

umarcor
Copy link
Collaborator

@umarcor umarcor commented Aug 21, 2022

Fixes #30

/cc @atorkmabrains

@mithro
Copy link
Contributor

mithro commented Nov 4, 2022

This pull request needs to be rebased before we can merge.

@atorkmabrains
Copy link
Collaborator

atorkmabrains commented Nov 4, 2022

@mithro this rebase and merge process will be extremely painful process due to the long standing PR list. We will have to redo many many times. Please give me admin access on this repo to finalize all changes.

@@ -48,36 +50,33 @@ jobs:
name: ${{ matrix.type }} | ${{ matrix.test }}

env:
PDK_ROOT: $GITHUB_WORKSPACE/rules/klayout
PDK: ${{ matrix.pdk }}
IMAGE: ghcr.io/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/test
Copy link
Member

Choose a reason for hiding this comment

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

is the image public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The image is neither public nor private, because it's not pushed to any registry. It's built in one step and used in the next step, both of them executed within the same job. Therefore, the name might be anything; it's local only.

However, it might make sense to eventually split building the image and using it, in order to avoid installing the dependencies in each job. That's what #55 is about.

Copy link
Member

Choose a reason for hiding this comment

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

ah make sense, curious if you consider caching it with docker save/load instead?

@proppy
Copy link
Member

proppy commented Nov 7, 2022

@mithro this rebase and merge process will be extremely painful process due to the long standing PR list

It shouldn't be a problem to rebase this change, it looks like the only other PR affecting .github/workflows/Tests.yml is #55 and it's still a draft.

@atorkmabrains
Copy link
Collaborator

@proppy The name change PRs are very extensive. We will have to do the rebase and merge many many times.

@proppy
Copy link
Member

proppy commented Nov 7, 2022

We will have to do the rebase and merge many many times.

I believe that github only mandate a rebase when it detect a conflict, see https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/about-merge-methods-on-github

@atorkmabrains
Copy link
Collaborator

@proppy I know that. Name change will definitely create conflicts. This a major change and work that I want to avoid.

proppy pushed a commit to proppy/globalfoundries-pdk-libs-gf180mcu_fd_pr that referenced this pull request Feb 6, 2023
add notebooks to plot the measurements against the simulation
@umarcor
Copy link
Collaborator Author

umarcor commented Apr 9, 2023

Closing per #30 (comment).

@umarcor umarcor closed this Apr 9, 2023
@umarcor umarcor deleted the umarcor/ci/container branch April 9, 2023 15:41
@atorkmabrains
Copy link
Collaborator

Thanks @umarcor Appreciate your work. Was really helpful.

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.

Create a custom image to use for our CI testing
4 participants