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

Include FileList describing files and directories (closes #2270) #2271

Closed
wants to merge 3 commits into from
Closed

Include FileList describing files and directories (closes #2270) #2271

wants to merge 3 commits into from

Conversation

merriam
Copy link

@merriam merriam commented Oct 10, 2022

This is a FileList that gives brief descriptions of the directories and files
involved in the project to help attract more contributors and readers of the
source code. This gives short explanations outside the range of documentation of those
only using the package and outside of what is available from an IDE or rustdoc. This
list saves an effort repeated by each new potential contributor.

@merriam
Copy link
Author

merriam commented Oct 19, 2022

Would someone rerun the checks? As this is a documentation only change, the CI system is the cause of any build issues.

@merriam
Copy link
Author

merriam commented Oct 25, 2022

Druid PR Blameless Report

Well, it has been two weeks. While not yet closed, it's blameless-report time!

What Was Expected: This is a small documentation update that touches neither the generated documentation nor the control flow. I expected a quick review and either acceptance or a 'please fix' comment. For example, a request to remove FileList.toml as irrelevant. The plan was then to submit updates about once a month until the project is fully described in self-contained text and html files.

What Happened: Pull request never completed Continuous Integration (CI). First, CI forced a wait for five days until a regular contributor allowed the pipeline to run. Second, the pipeline failed twice due to coding errors: one in the master branch (unused return value in druid-shell/src/backend/mac/window.rs:377:17) and once in a dependency (borrow does not live long enough, in mdbook, a rust-lang project, mdbook-0.3.7/src/renderer/html_handlebars/helpers/navigation.rs:155:25). The CI pipeline failed. After an additional six days, I posted on the PR asking someone authorized to rerun the CI tests. After an additional four days, we go to this blameless report.

What are the likely issues: There are three possible issues from my limited visibility:

  • The CI system faces challenges, possibly be being hosted on a Microsoft service and out the control of core maintainers. It may be grudgingly coaxed and worked around. Supporting evidence for this would be about half the recent CI jobs failing, (1), CI taking about ten minutes of wall clock time (2), and a large number of ignored errors and warnings (2 again). This would suggest the pull request is not unwelcome, just that it caught the project at a bad time.
  • The project actually embraces a “Cathedral” system over the “Bazaar”. That is, the project would like substantial contributions by a small, well known group of contributors. These projects are usually run as a consortium project by a discrete number of companies as was Apache. In contrast, a Bazaar project has a larger number of contributors, often making less substantial contributions. This would suggest the pull request is unwelcome, and it would be best to leave the project in peace.
  • Developers feel the PR adds no value and wish to let it passively die without comment. This would contradict Ralph Levien’s comments in Zulip.

What are the next steps?

Wait a few days, see if the CI ratio improves significantly, and, if not actions, assume a Cathedral, close the pull request, and ghost the project.

@raphlinus
Copy link
Contributor

Sorry this project wasn't what you expected.

@raphlinus raphlinus closed this Oct 25, 2022
@dvc94ch
Copy link

dvc94ch commented Oct 25, 2022

I can understand that adding a list of files would quickly get out of date, imposes a maintenance burden and probably no one will read. Although I have no affiliation with druid, this is a drive by opinion. Seems like a constantly failing ci is a valid issue in your blameless report.

@merriam merriam deleted the filelist branch October 25, 2022 03:22
@PoignardAzur
Copy link
Collaborator

Is this a standardized format? Is there a specification somewhere?

I agree that the fact this PR was left to rot is an organizational issue; but that aside, I don't really get the point of this PR. It seems like the file descriptions in FileList.txt would be better suited for an ARCHITECTURE.md file.

https://matklad.github.io/2021/02/06/ARCHITECTURE.md.html

@richard-uk1
Copy link
Collaborator

richard-uk1 commented Oct 25, 2022

As someone who is interested in OS organization (the verb), I think this PR is interesting. Here are my thoughts.

The problems with CI were caused by

  1. us converting clippy warnings to errors, and clippy adding new warnings without it being a breaking change
  2. rustc starting to reject code from the handlebars crate because of a bugfix leading the compiler to reject more programs (Latest nightly (rustc 1.64.0-nightly (62b272d25 2022-07-21)) introduces a new borrowcheck error in previously compiling mdbook 0.4.20 rust-lang/rust#99591).

And the issue with the ticket overall is that no-one replied.

(1) is an issue for us I think. I wonder if we should make a rule that in the CI the clippy check is optional, and then in the review process say that you only have to fix clippy errors in code you've added/changed as part of the PR. (2) is just bad luck and can be fixed by updating mdbook (I haven't yet checked if this is fixed in master/main now).

The issue with no-one replying is IMO actually more complex and interesting. What I find with open source projects is that reviewers (myself included when I'm taking a reviewer role) tend to ignore things they don't understand, or perhaps more precisely can't understand quickly by looking at the text of the issue/PR/ticket/etc. When I'm looking at my notifications, I will go through them all and if I see one where I think "this isn't really something I know about and I don't have much to add" then I just ignore it. The problem comes when everyone does the same thing, because the cumulative effect of all these perfectly fine micro decisions is a not perfectly fine outcome - namely that someone who took time to contribute is ignored.

The problem specifically with druid at the moment is that because the main developers' efforts are focused elsewhere and druid itself is likely to change a lot (or be superseded) in the future, there currently isn't anyone who has final responsibility for handling open issues. Not that I think there necessarily should be: with druid I'm not sure the effort required to be a 'reviewer of last resort' - a job that takes time - is worth the time at the moment.

My personal recommendations for all of this are, firstly for the project

  • We put a notice on the repo stating that, while the crate is usable and maintained, it is not being actively developed at the moment, and so it might take time for replies. We should include a link to the Zulip instance more prominently because that is the most active community around druid, and is likely to remain so in the future.
  • We should allow people to merge PRs that break clippy when the breakage was introduced by an updated clippy rather than by the PR.

For @merriam, I would recommend

  • Adding more detail about what this PR future PRs add to a project. In this PR, you say

    gives brief descriptions of the directories and files involved in the project to help attract more contributors and readers of the source code

    but this still leaves some questions unanswered: is FileList a standard? why do this rather than write a short markdown doc describing the code structure?

  • Consider opening an issue to describe a problem before crating a PR to solve it. This allows the problem space to be discussed prior to work on a PR, and will give you more certainty as to whether it is likely to be merged before you start work.

    You don't have to do this - I recently wrote a PR where I broke this rule and a not inconsiderable amount of my time will be wasted when the PR is inevitably rejected. But I don't mind because I had fun hacking on the codebase, and I was aware a priori that the work might be rejected. Just know that rejection is more likely if you do the PR without any prior contact.

Thanks @merriam for the report, which IMO was a material contribution to the project. I'd be interested to hear either your or anyone else's criticisms/thoughts on what I've wrote here.

@merriam
Copy link
Author

merriam commented Oct 27, 2022

* Consider opening an issue to describe a problem before crating a PR to solve it. This allows the problem space to be discussed prior to work on a PR, and will give you more certainty as to whether it is likely to be merged before you start work.

Thank you for advice here. I did open #2270 and let it sit for a week or so with no comments. This does make it difficult to be sure to answer questions about standards or preference of generated reports over architecture markdown documents. I had also reached out on Zulip to ask if contributions were accepted if they added some value instead of only accepted after reaching some level of perfection. This wasn't a blind pull request.

Only after the pull request and its branch were deleted did people come out to comment why the grapes were probably sour anyway. It is fine to have a Cathedral project where the contributors are a small, cozy bunch. Great software has been built that way, and, for many project, it is the fastest way to make new software.

Have a wonderful day.

-- Charles

@PoignardAzur
Copy link
Collaborator

Only after the pull request and its branch were deleted did people come out to comment why the grapes were probably sour anyway.

Well, that seems a bit passive-aggressive. I was asking an honest question, not trying to justify why the issue was abandoned.

@merriam
Copy link
Author

merriam commented Oct 27, 2022

Well, that seems a bit passive-aggressive. I was asking an honest question, not trying to justify why the issue was abandoned.

I am sorry as you took it as passive aggressive. I point out that the question was raised after the decision about the pull request had been made. I was not trying to cause offense.

@raphlinus
Copy link
Contributor

My response was terse, but I do want to add that I'm aware there are some organizational issues and hoping to really improve things, so this discussion has been useful. I'm experimenting with "office hours," which will be Wednesday mornings at 8am Pacific time (starting next week), and a topic is definitely project governance. You're more than welcome to bring your experience, it would help catalyze some discussion. And of course I fully understand if you choose not to.

@merriam
Copy link
Author

merriam commented Oct 28, 2022

You're more than welcome to bring your experience

Thank you for the kind offer and for affirming your wish to bring improvement. I cannot think of any relevant information to add.

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

Successfully merging this pull request may close these issues.

5 participants