From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 13 May 2019 07:46:21 -0700 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 35BF93082206; Mon, 13 May 2019 14:46:21 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-237.rdu2.redhat.com [10.10.123.237]) by smtp.corp.redhat.com (Postfix) with ESMTP id 27AB210018E0; Mon, 13 May 2019 14:46:19 +0000 (UTC) Subject: Re: [edk2-devel] RFC for Edk2-Library To: devel@edk2.groups.io, sean.brogan@microsoft.com, Michael D Kinney References: <19931.1557456493073446522@groups.io> From: "Laszlo Ersek" Message-ID: <5f17bc94-e175-8cb8-13c5-837ea0408203@redhat.com> Date: Mon, 13 May 2019 16:46:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <19931.1557456493073446522@groups.io> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.47]); Mon, 13 May 2019 14:46:21 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/10/19 04:48, Sean via Groups.Io wrote: > 1. Agree on the name but not sure whats better. i don't think it > should be edk2-tools because the idea of this is to be a library of > support code but not the tools themselves. This limits dependencies > and keeps the library free of business specific logic. The second RFC > will be for a new repo that is more like edk2-tools. So maybe this > could be edk2-tools-library? > > 2. Future direction (edk2 having a dependency on this package). I > don't think this library will achieve my goals if it doesn't start to > pull in support libraries from basetools. The simplest example is the > parsers. It would seem foolish to have two copies. It would be great > to have one set of parsers that could be used by tool developers as > well as the edk2 build process. Opening up those tools would make > writing edk2 analysis tools much easier/faster/better. > > 3. Pip/distribution/versioning. I would definitely not want my > version of this pip module dependent on my OS distro and version. This > will be something tied to the edk2 platform i am building. These are > things that are somewhat stable but i would expect more churn than an > OS and different platforms will have different needs. i also don't > want to sign up for working with OS vendors to get this into their > package management. With python 3.5 and newer there is a built in > concept of virtual environments (venv). This is how i would handle > this problem. The Pip modules and their dependencies do not impact the > global packages. Only the virtual environment gets updated and a > virtual environment can be trivially created/deleted. This is also > expected if you are building platforms that might be running different > versions of edk2 so its a good idea to create a venv for each code > tree on your system and activate that venv when doing builds in that > code tree. OK. I think we'll just have to package the right version of the new python dependency, downstream, eventually. > 4. Squash merge/PR process. We discussed this at length at plugfest > and unfortunately i don't think there was any silver bullet. Patchsets > and the edk2 process today are a relic of emailing patches. That's your opinion ("relic"), which I respect. It's not a fact though. There are large communities that email patches (with git-send-email) because that's the most scalable and proven approach for them. It's not because they are prevented from using anything better. For these people (including me), there *is* nothing better. I *have* contributed to projects with a GitHub.com-only workflow. Red Hat contributes to, and maintains, several such projects. git-send-email is still better, in my opinion. It's not perfect, of course. If the TianoCore community perceives the patch email based workflow limiting, I will not try to prevent the community from adopting something else. But I would like to see some values preserved. (Unless contributors clearly state that they don't care about those values.) > When you move to pull requests managed by a server with policy and > automation i think the only feasible solution is squash merge. Now i > don't understand why squash merge means bad commit messages and lost > info. Please refer to the following patch series, as an example. [edk2] [PATCH 0/4] MdePkg/BaseSafeIntLib: fix undefined behavior in INT64 Sub/Add/Mult http://mid.mail-archive.com/20180215183638.18578-1-lersek@redhat.com https://lists.01.org/pipermail/edk2-devel/2018-February/021476.html It was committed as: 1 54c7728a0465 MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Sub() 2 41bfaffd1309 MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Add() 3 8c33cc0ec926 MdePkg/BaseSafeIntLib: clean up parentheses in MIN_INT64_MAGNITUDE 4 75505d161133 MdePkg/BaseSafeIntLib: fix undefined behavior in SafeInt64Mult() If you squash these patches together, you will end up with a single patch that modifies three independent functions, and rewrites a macro, at the same time. In addition, you will either lose the details of the individual commit messages, or else you will end up with a humongous and incoherent commit message. For example, the reasoning in commit #1 is distinct from the reasoning in commit #4. In addition, if the series had caused a regression, then git-bisect would have had a lot worse granularity in locating the culprit patch. With the above patch set structure, git-bisect would have told us what function or macro to start investigating. Squashed together, we'd have only gotten a *set* of functions to investigate. This is not a theoretical problem. 1000-line feature patches are still all too common in edk2. They are basically impossible to analyze in a bug hunt, especially for someone that has not originally authored the feature. (Analyzing someone else's code for issues is the norm in open source development.) > The idea is that a single PR should be a single feature. Yes, I agree 100%. And implementing a single feature may take tens of patches, which should not be squashed together. > When it gets squashed the commit message should reflect the feature > and be of high quality. Commit messages are entirely expected to go into implementation details. A high level feature description is entirely justified and even necessary, but it belongs in the feature request ticket, a wiki article, and/or a standalone design document. > If the PR was too big and contained numerous features then the PR > should be split up. Fully agreed. However, the above example justifies neither more than one PR, nor squashing patches #1 through #4. > Squash merge allows your automation to easily guarantee > bisect-ability, I don't understand. If I develop patches #1 through #4 logically separated from each other, but then I push them as one big squashed commit, how will any kind of automation conjure up the original patch boundaries? > build-ability, and that each commit will pass the requirements. Having > one continuous PR optimizes your review process/comment tracking, > etc. I would be strongly opposed to opening new PRs for every new > "version". I don't insist on a new PR for every version of a patch series, as long as every version of the patch series is preserved forever, as a *complete* topic branch, with context-sensitive review comments. I'd like contributors to edk2 (both proven and prospective) to express their preferences on this. If it's demonstrated that I'm trying to force workflow specifics on edk2 that are not welcome by the majority, I'll shut up. Thanks Laszlo > > Thanks > Sean > > > >