Add python2 info to documentation
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- update-docs
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
adding @ashoat as blocking since this is documentation: https://www.notion.so/commapp/Diff-Review-Rules-addfc3a41b324c7d8ba636dd9dcefeba
Comment Actions
I'm not sure how specific we want to be here in the dev_environment doc since we tend to keep things to more of a "recipe"... but I think we should provide a little more context since this is a (hopefully) temporary workaround. Maybe noting somewhere that this is only necessary for ARM-based Macs?
Context: @redux-devtools/cli depends on sqlite3 (the npm package) which doesn't have prebuilt binaries for ARM-based Macs. The sqlite3 native build process depends on a script written in Python 2.x. Up until macOS 12.3, Python 2.7.x was "included in macOS for compatibility with legacy software." However, now that Python 2.7.x is no longer included, the sqlite3 build fails, which causes yarn cleaninstall to fail, etc.. and leaves us with a broken dev environment.
It's not possible (or difficult/unsupported) to install Python 2.x using Homebrew--which we use to manage other "system" dependencies. It is possible to install Python 2.x using something like pyenv, but that adds another layer of complexity. We decided for the purposes of a temporary workaround, and to get us back to the pre macOS 12.3 world as soon as possible, that the best bet was to use the Python 2.7.18 64-bit macOS installer from the Python.org releases page.
| docs/dev_environment.md | ||
|---|---|---|
| 568 | Might be being pedantic, but should we specifically point them to the macOS 64-bit installer link on the page? | |
Comment Actions
Agree with both of @atul's comments. For the record, the rules about when to add me should only be applied after the first round of review. The intention of removing me from blocking every review is to free up my time... we'd be moving backwards if we started putting me on the first round of reviews. We want to keep the rule of "Ashoat generally is a second round reviewer", but also pull back on "Ashoat needs to be on the second round of every single review"
Comment Actions
Great, I'll update the docs. I wasn't sure how much detail was really needed here. I'll add a bit more context.
| docs/dev_environment.md | ||
|---|---|---|
| 568 ↗ | (On Diff #10911) | https://stackoverflow.com/questions/60298514/how-to-reinstall-python2-from-homebrew -- it's possible but pretty non-standard. I feel like they effectively removed it. |
Comment Actions
We should also either stick to Python 2.x or Python 2.7.x throughout. Python 2.7 came out in like 2010, so it's like the relevant version of Python 2, but can probably just say Python 2.x throughout
| docs/dev_environment.md | ||
|---|---|---|
| 566 ↗ | (On Diff #10911) | nit: should wrap in backticks |
| 566 ↗ | (On Diff #10911) | nit: cut this outer period |
| 566 ↗ | (On Diff #10911) | nit: should probably be install vs download |
| 566 ↗ | (On Diff #10911) | nit: can probably remove "process" |
| 566 ↗ | (On Diff #10911) | nit: maybe "manually" instead of "locally"? |
| 568 ↗ | (On Diff #10911) | nit: we can say "supported" instead of "possible" |
| docs/dev_environment.md | ||
|---|---|---|
| 564 ↗ | (On Diff #10911) | Should mention that this is specific to ARM-based Macs. There are pre-built sqlite3 binaries available for x86 Macs, so the build step is skipped altogether. |
| 568 ↗ | (On Diff #10911) | It's not really a recommendation as much as it is an instruction? |
| 568 ↗ | (On Diff #10911) | |
Comment Actions
This is appearing on my queue when it really should be on @atul's and @benschac's. We're getting in this situation because @atul isn't hitting "Request Changes"... which means if I remove myself from the revision, it will go to @atul's queue instead of @benschac.
@atul, can you please make sure to always hit either "Request Changes" or "Accept" after leaving comments going forward?
Will hit "Request Changes" here for you again this time...
| docs/dev_environment.md | ||
|---|---|---|
| 564 ↗ | (On Diff #10911) | which doesn't have prebuilt binaries for ARM-based Macs. But, I can be a little more straight forward. |
Comment Actions
Intentionally removed myself as a reviewer since I didn't/don't intend to do a full review, just left some unsolicited comments since I had context on the Python 2 issue.
I've chatted with @atul and @varun and we're going to try moving away from @atul handling reviews of docs diffs to @varun handling it.
Comment Actions
I still feel strongly that in both of your more recent comments, you should have requested changes. Otherwise the diff does not go to the author's queue.
| docs/dev_environment.md | ||
|---|---|---|
| 564 ↗ | (On Diff #10958) | Even though we don't specify version numbers in the header for other dependencies (eg node/yarn), I think it might make sense in this situation. Python 2 is particularly distinct and incompatible with Python 3, so it might be worth emphasizing that we don't want to install "Python" generally, but a specific version of Python 2 for a specific reason? Don't feel strongly, just a thought. I could see myself setting up the dev_environment, seeing "Python," and just skipping past since "I already have Python installed." I think we should also include in the header that this is only for "ARM-based Macs." We refer to them as "Apple silicon Macs" elsewhere in the dev_environment doc, so that's probably what we should stick with. |
| 566 ↗ | (On Diff #10958) | nit: We should capitalize "Python" |
| 566–570 ↗ | (On Diff #10958) | I think we can break this into three "chunks"? A. Brief context explaining which versions of macOS require installing Python 2 manually. Provided a rough draft of what it might look like above... feel free to use/ignore/rework however you think works best |
| 568 ↗ | (On Diff #10958) | nit: We should capitalize "if" |
| docs/dev_environment.md | ||
|---|---|---|
| 566 ↗ | (On Diff #10958) | |
| docs/dev_environment.md | ||
|---|---|---|
| 574 ↗ | (On Diff #10970) | remove |
Comment Actions
Please pay attention to the headers you're nesting new sections into, and how those headers relate to each other. If you don't have an idea of how these sections break down, you should pause any docs work and first try to understand how the sections relate
| docs/dev_environment.md | ||
|---|---|---|
| 333 ↗ | (On Diff #10994) | This shouldn't be in the Configuration section, it's a prereq and has no configuration steps |