google / globalfoundries-pdk-libs-gf180mcu_fd_pr

Primitives for GF180MCU provided by GlobalFoundries.
https://gf180mcu-pdk.rtfd.io
Apache License 2.0
45 stars 27 forks source link

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

Open proppy opened 1 year ago

proppy commented 1 year ago

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.

atorkmabrains commented 1 year ago

@proppy and @mkkassem

Please check the usage in this document: https://github.com/mabrains/globalfoundries-pdk-libs-gf180mcu_fd_pr/blob/main/cells/klayout/pymacros/README.md

atorkmabrains commented 1 year ago

@proppy About regression, please find more information here: https://github.com/mabrains/globalfoundries-pdk-libs-gf180mcu_fd_pr/blob/main/cells/klayout/pymacros/testing/README.md

proppy commented 1 year ago

@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 commented 1 year ago
proppy commented 1 year ago

I think that if you:

  1. take the instruction in https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/issues/70#issuecomment-1311156971
  2. force push to https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/pull/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 commented 1 year ago

@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 commented 1 year ago

@atorkmabrains so you'd prefer me to do the changes highlighted in https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/issues/70#issuecomment-1311156971 and https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/issues/70#issuecomment-1311302640 rather than giving it a go? (just checking if I interepreted correctly your last comment)

atorkmabrains commented 1 year ago

@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 commented 1 year ago

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: https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/issues/70#issuecomment-1311302640).

@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 commented 1 year ago

@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 commented 1 year ago

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 https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/issues/70#issuecomment-1311156971 and https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/issues/70#issuecomment-1311302640)

atorkmabrains commented 1 year ago

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