Introduce a PORTING.md project doc for contributors by wrowe · Pull Request #11721 · envoyproxy/envoy

wrowe · 2020-06-23T20:46:41Z

Commit Message: Introduce a PORTING.md project doc for contributors
Additional Description:

Soliciting contributions to build on this after it is merged
to master from both Microsoft contributors and OS/X champions.
It is also worthwhile to solicit more contributions to build
on these notes from the Apple iOS and Android porting efforts.

Signed-off-by: Sunjay Bhatia sunjayb@vmware.com
Co-authored-by: Sunjay Bhatia sunjayb@vmware.com
Signed-off-by: William A Rowe Jr wrowe@vmware.com
Co-authored-by: William A Rowe Jr wrowe@vmware.com

Risk Level: n/a
Testing: Based on local and RBE development
Docs Changes: n/a
Release Notes: n/a

- Soliciting contributions to build on this after it is merged to master from both Microsoft contributors and OS/X champions. - It is also worthwhile to solicit more contributions to build on these notes from the Apple iOS and Android porting efforts. Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

wrowe · 2020-06-23T20:50:30Z

@davinci26 @nigriMSFT your feedback would be helpful if we've missed anything significant. @mattklein123, @htuch and @lizan since you have all looked into a number of windows regressions, your input is also helpful.

It would be good to come to terms on how we want to structure these notes and where we want them to live, and get them out there for all contributors to consume as we turn on RBE test CI pipeline with PR 11107

lizan · 2020-06-23T20:53:05Z

@zuercher for comment on macOS port

wrowe · 2020-06-23T20:53:55Z

@sunjayBhatia for follow

htuch

Thanks for starting this, a few high level clarifications to get started.

htuch · 2020-06-24T01:03:57Z

PORTING.md

@@ -0,0 +1,128 @@
+## Envoy Porting


Should we cover cross-architecture porting here? E.g. ARM, PowerPC, etc.

Absolutely. A 32-bit android PR reiterated this point today. We should likely break this down into subject areas that aren't as much "windows specific" as "architecture variances", e.g. 32 bit android or arm or linux, vs P64 Windows, vs ILP64 linux. Each of these should have short breakouts, but the suggestions writ large should focus on the individual commonly misunderstood subjects and how they should be coded to remain portable.

htuch · 2020-06-24T01:05:12Z

PORTING.md

@@ -0,0 +1,128 @@
+## Envoy Porting


What is the goal of this doc? To capture some known port issues, to provide best practices for not breaking ports, to advise on how to add a new port?

My #1 goal in authoring PORTING.md is to help regular contributors to look beyond baked-in Linux x86_64 assumptions and anticipate what is needed to write portable C++ code. To that end, it needs be structured to help resolve CI pipeline breakage, whether that happens to their submission in the OS/X or Windows ports, or ARM 32 or 64, or Android or other future ports.

Should the key takeaways just become part of STYLE.md? I get that we want a more details doc providing an explanation of "why?" rather than just "how", but it would be helpful to only be able to read STYLE.md and have the rules of the road here.

I would leave it to the core maintainers to decide which suggestions to adopt. For example, suggesting number of bytes or number of elements (.size() or .count()) used for a storage class in-memory should always be represented as size_t/ptrdiff_t, while sizes of storage class on-disk should be represented as off_t, because that's what ISO stdc++ does. Fighting with the C++ standard library is probably the silliest windmill we could tilt at.

@envoyproxy/maintainers for thoughts on this. I think we'd be happy to take some basic suggestions like this. PR welcome, we can bikeshed there.

htuch · 2020-06-24T01:06:02Z

PORTING.md

+* Mac OS/X port 
+* Windows Win32 API port
+
+## Troubleshooting Common Porting Issues


Are there some best practices or advice for developers on how now to break Windows? See for example our other thread on autoconf deps.

Although it is largely secret-sauce, much of this goes back to K&R 1st edition. I've already added two specific links in the docs, one which covers the vast array of compilation differences between different compilers and the other which Microsoft themselves calls out the most commonly encountered quirks of their cl.exe compiler. We should add all such related and helpful links (rather than overburden what will become a long-enough document, as-is.)

Things like working around GNU autoconf-managed projects needs to be in an altogether different section than linkage compatibility, bazel compatibility etc.

And on the subject of 'examples' I do think that adding links to specific lines of commits where the specific code was fixed once upon a time so readers can see a before-and-after picture would be helpful to each of the particular problems. But getting this doc into everyone's hands as we kick off #11107 takes higher priority.

One worry I have is that the document is long, which you point out, and most folks won't have time to read. Ideally we have the takeaway bullet points early on or in some easy to scan for manner, e.g. call-out boxes, so that the relevant info can be ingested in a TL;DR world.

I expect it is a fraction of how long it may end up... I'm looking at this from github's document.md rendering for the first time, and we will absolutely format it for clarity and "skim-ability".

davinci26 · 2020-06-24T04:47:46Z

I think this is an awesome doc! I think the information here is really valuable and probably it is worth be published separately as well as it can help other porting efforts.

davinci26

A general remark here. Depending on the goal that we have for the doc we may want to include some examples

davinci26 · 2020-06-24T04:56:17Z

PORTING.md

+
+## Specific Coding Issues
+
+Most porting issues can be summarized in a handful of assumptions to be avoided.


Do you think would make sense to add some subheaders here and split into these broad categories:
e.g.

C++ standards

Word size

C Runtime

System calls

The scenario that I am thinking is that a contributor might want to skim-through the doc while having a specific issue and some subheaders might make it more easier to find the relevant paragraph or section to their problem

Yes I believe we are going to need further sub-headings. What the logical groupings are, I haven't worked out and welcome patches and suggestions.

davinci26 · 2020-06-24T05:00:34Z

PORTING.md

+
+Many tests rely on command line scripting or tool invocation. Bazel and Envoy rely heavily on bash scripts, executing
+on Windows via msys2. Inherent discrepancies between the msys2 execution environment and a typical bash shell can
+cause confusing errors. Be sure to use cmd.exe on Windows in any test scripts that intend to create symlinks.


Should we have a separate section for testing and add there:

cmd for symlinks

googletest regex

googletest nice mocks on base classes

Sub-sections, yes as noted above.

wrowe · 2020-06-25T05:30:50Z

I think this is an awesome doc! I think the information here is really valuable and probably it is worth be published separately as well as it can help other porting efforts.

I'd be happy to pursue that, including offers of co-authoring help and contributions, but let's build this document first over 6-12 weeks :)

wrowe · 2020-06-25T05:35:00Z

/azp run envoy-presubmit

azure-pipelines · 2020-06-25T05:35:05Z

Commenter does not have sufficient privileges for PR 11721 in repo envoyproxy/envoy

wrowe · 2020-06-25T05:35:20Z

/retest

repokitteh-read-only · 2020-06-25T05:35:22Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #11721 (comment) was created by @wrowe.

see: more, trace.

wrowe · 2020-06-25T05:42:03Z

To add a top-line comment for all reviewers, the goal in this document is not to thoroughly delve into each possible porting problem, but to highlight the most commonly observed problems, especially at the Envoy project specifically, and add copious links to where the reader can learn more.

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

DEVELOPER.md

htuch · 2020-06-25T19:08:13Z

PORTING.md

+restrictions than on the typical Linux/clang tool chain. These limits include;
+
+* The individual path elements for -Inclusion (this restricts the total path name lengths available to the project).
+* The length of the embedded command line stored in the debug info record of the COFF object file (48kb on Windows).


How can developers action on this to prevent running into this situation?

We haven't worked out that solution yet. It's noted here in case Windows developers are observing strange debugging behavior, it isn't something we expect to see in CI. The way that Bazel assembles works with insane lists of inclusions suggests we won't be able to work around this. We can probably pull the comment if we don't see issues in CI or from developers, since there isn't anything that Envoy itself can realistically do.

Note to self to inject bazel advice to Windows builders here, as mentioned in #12098

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com> Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com> Signed-off-by: William A Rowe Jr <wrowe@vmware.com> Co-authored-by: William A Rowe Jr <wrowe@vmware.com>

lizan

Just a couple of nits

DEVELOPER.md

PORTING.md

Signed-off-by: William A Rowe Jr <wrowe@vmware.com>

stale · 2020-07-19T00:36:11Z