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

cells/klayout/pymacros/README: should document how to use pcells #70

Open
proppy opened this issue Nov 9, 2022 · 12 comments
Open

cells/klayout/pymacros/README: should document how to use pcells #70

proppy opened this issue Nov 9, 2022 · 12 comments
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@proppy
Copy link
Member

proppy commented Nov 9, 2022

Expected Behavior

https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/tree/main/cells/klayout/pymacros README should document how to generate gds using klayout and how to regenerate the tests in https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/tree/main/cells/klayout/pymacros/testing.

Actual Behavior

https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/tree/main/cells/klayout/pymacros assume developers/contributors are familiar with the process to generate pcells using klayout and update testcases gds.

@mkkassem mkkassem added the documentation Improvements or additions to documentation label Nov 9, 2022
@proppy proppy changed the title cells/klayout/pymacros/README: should document how to generate pcells cells/klayout/pymacros/README: should document how to use pcells Nov 9, 2022
@atorkmabrains
Copy link
Collaborator

@atorkmabrains
Copy link
Collaborator

@proppy
Copy link
Member Author

proppy commented Nov 11, 2022

@mkkassem @atorkmabrains I think you can easily create a separate pull request for the pcell changes this way:

# clone upstream repo
git clone https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr
cd globalfoundries-pdk-libs-gf180mcu_fd_pr
# add your fork remote
git remote add mabrains git@github.com:mabrains/globalfoundries-pdk-libs-gf180mcu_fd_pr.git
git fetch mabrains
# create feature branch
git checkout -b improve-pcell-docs
# checkout (only) pcells changes
git checkout mabrains/main cells/klayout/pymacros
# create a new collapsed commit for (only) pcells changes 
git commit -m 'cells/klayout/pymacros: improve documentation'
# push back to your fork
git push mabrains improve-pcell-docs
# create the pull request

@atorkmabrains
Copy link
Collaborator

  • @proppy I have discussed that with @mithro before. That can't be done. Primitive renaming is a lateral change that affects everything and if you take one part and leave the other parts of the PDK unchanged, it will break the PDK. And there is another issue, doing PR will not work as well as the amount of changes are massive. And github doesn't allow that. I have shown this to @mithro and @mkkassem. That's why I was asking you guys to get the primitives renaming PRs in first before we change anything.

@proppy
Copy link
Member Author

proppy commented Nov 11, 2022

I think that if you:

  1. take the instruction in cells/klayout/pymacros/README: should document how to use pcells #70 (comment)
  2. force push to Updating primitives names in cells directory #32 branches (https://github.com/mabrains/globalfoundries-pdk-libs-gf180mcu_fd_pr/tree/primitives_names_part1)
  3. mark it as fixing this issue

That should be enough to provide a coherent set of change to review and merge (that include both naming change and appropriate documentation update to evaluate it).

@atorkmabrains
Copy link
Collaborator

@proppy I have all updates made in mabrains branch. I advise you to just replicate what you have here with what's in Mabrains.

@proppy
Copy link
Member Author

proppy commented Nov 11, 2022

@atorkmabrains so you'd prefer me to do the changes highlighted in #70 (comment) and #70 (comment) rather than giving it a go? (just checking if I interepreted correctly your last comment)

@atorkmabrains
Copy link
Collaborator

atorkmabrains commented Nov 11, 2022

@proppy My advise is basically to replace what you have here with what's in Mabrains fork. And after that let review one by one. If there are any other changes required, we could handle them one by one. This is just to start with a clean slate. If you have doubts in my proposal, please go ahead and check the diff against what's in Mabrains. I'm not responsible of any issue if you didn't update to match Mabrains repo.

@proppy
Copy link
Member Author

proppy commented Nov 11, 2022

And after that let review one by one. If there are any other changes required, we could handle them one by one.

I'd prefer for us to work with updating the existing pull request as I suggested: swapping the ownership of repo is not really desireable as we'd loose all the history of our conversation and get into funky redirections.

I'm not responsible of any issue if you didn't update to match Mabrains repo.

@mkk asked me to spell out the instructions to update the repo, on each issues (ex: #70 (comment)).

@atorkmabrains, I guess I could also go ahead and update them directly, if you don't feel confortable or doubt those the said instruction, as long as @mithro, @QuantamHD @mkk agree it's the right thing to do.

@atorkmabrains
Copy link
Collaborator

@proppy Please do whatever you need at your end. But make sure that at the end that you match what I have on Mabrains side.

I believe you misunderstood me btw, what I'm saying is that you take what I have and push it to Google repo. Once that's done we could start adding PRs on Google side.

Please try to do that at your end.

@proppy
Copy link
Member Author

proppy commented Nov 11, 2022

what I'm saying is that you take what I have and push it to Google repo

The two repository don't really have the same granularity of changes:

While the later is fine while working on an individual changes on a personal fork, it's not desirable to have this level of granulary for a published history as it doesn't provide any context (in terms of PRs / issues) for the changes that are being made.

With this in mind, my recommendation would be for us to update the individual PRs that you already have in flight in https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/pulls based on the changes you have in https://github.com/mabrains/globalfoundries-pdk-libs-gf180mcu_fd_pr/commits/main after splitting and flattening them using git commands similar to the instructions provided in each issues (ex: for this one #70 (comment) and #70 (comment))

proppy pushed a commit to proppy/globalfoundries-pdk-libs-gf180mcu_fd_pr that referenced this issue Feb 6, 2023
@atorkmabrains
Copy link
Collaborator

@FaragElsayed2 We need to follow up on this in efabless version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants