-
Notifications
You must be signed in to change notification settings - Fork 184
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
Should we require a separate fpm package first before standardizing? #658
Comments
Note that |
About not being able to use fpm to test PR, you actually can but it's just undocumented, you just need to run the
I wonder if the CI can create something like a "release" when a PR happens (in its fork of course), so you just download the fpm package, unpack it, and test. Or maybe just documenting the process above in a PR template (which I'm extrapolating, does it exists?) |
I totally support some kind of fpm-based development of stdlib features, I was hoping something like a central wiki for functions such as these That is exactly what I tried to create for this very topic with Perhaps something like a sponsored set of moderated categories based off For this topic, maybe the model suggestions would have looked like
and functionality could be listed and voted on by people actually indicating
people could then submit their fpm packages for consideration and discussion? So basically, I think fpm is a near-perfect tool for creating various What is missing that this is not the case? Would a "request for papers" Is there some model other languages use that would be useful? |
Unfortunately, this is currently the default for most PRs at stdlib taking several weeks to months until they get reviewer attention and not a criteria to judge a PR. Regarding fpm packages for PRs, I'm happy to support this idea and have done so in the past, especially for strings. From my experience with those was not a big difference in review time nor adaption so far whether the project existed as an fpm package before. On the other hand, creating, maintaining and establishing an fpm package is a lot of hard work, especially maintaining the CI testing, automated documentation and maybe the fypp processing puts a huge demand on the contributors. At least for me pushing a project on GitHub means a significant time commitment for maintenance, working with reviewers on a PR of course requires also effort, however this is usually only coding rather than building reliable and long-lived software infrastructure. So while I'm in principle in favor of this idea, I worry it might hamper the pace of developing stdlib even further. |
I overall share @awvwgk's opinion. It seems a good idea at a surface, but I don't think it would help in practice.
I think this is an incorrect premise to start with. The problem with the Fortran standard and the committee is that there are features that are insufficiently tested and/or well-thought-through, and the committee requires backward compatibility in most cases (understandably so). This is not the case with stdlib. As we discussed many times before, we have an experimental level and stable tiers of features. Anything in experimental (currently everything) is subject to change. Features in stable are really what's comparable to standardized language features.
For example, I at least play a little bit for any feature PR that I review, unless I indicate in the review that I didn't try it but seems reasonable. In my view, it's really when features are committed in experimental that the real playing and testing begins, not the PR stage. I think we should be fast and loose with feature PRs to experimental (Is it in scope? Yes. Does it seem useful? Yes. Is something obviously incorrect about it? No. OK, let's merge it.), and only very conservative at the stage when we consider features for the stable tier. Those will obviously need to show a track record of use in other packages. So, if I'd recommend any change, it's for people to review, merge sooner, don't wait for approval from many people, and iterate in short cycles. If we realize later that an API or an implementation should be improved, we can do it. |
Yes, it's all about reducing the burden on both stdlib maintainers, and stdlib users and reviewers (often times but not always the same people). I can see only two ways forward:
Right now we do not do either, and that is what I object against. I am fine with either one approach, and Milan is right that we have discussed initially to use experimental precisely as a test project to make it easy for people to try out and use, and we reserve full right to completely change the API or even remove the functionality. So let's get back to our original plan, and merge much quicker to stdlib. The other part of the problem is that stdlib seems to be missing a single person who would be in charge of this, and ensure PRs get reviewed in a timely manner and merged and actively pushing after the goal/vision of stdlib. I can't be that person, as I am already that person for LFortran. Is there anybody who would be interested for this role? @awvwgk, @milancurcic, @14NGiestas, @urbanjost and others? |
I'm at capacity and do my best. Yes, ideally there should be a single person that's consistently on top of it, but it may be difficult to find. Until we do, perhaps we just need to broaden the team of "partial maintainers". |
Currently, stdlib PRs require approval from 2 maintainers (members with write access) for it to be merged. This number of 2 was arbitrary when I set it a long time ago. And nowadays we often get approval from reviewers without write-access, which doesn't count toward the required approval quota. Should we lower this requirement to 1 approval, with the hope of PRs getting merged faster? |
Yes, a single person in charge is the best. Until we can find such a person, then let's do a collection of people as partial maintainers (as we do now) and let's do our best not to delay things with "indecision". (I realize my objections partly caused the delay, but until we have a single person to push for this, please don't get discouraged if I am critical. Merging faster is a higher priority for me than my milder objections, which I still think was important to write.) |
The main issues for My main concern is that if we merge quickly feature PRs after a "light" review, these feature PRs that might be half backed and full of (hidden) issues, will remain as they are in Currently I already integrated a few features (e.g. logger, some stat functions, sorting procedures) in my main projects, and use daily almost all features through Therefore, while slow, I prefer to have well though and backed features, even in the experimental space. Lowering the number of approvals for merging could be also a problem. For example, there is currently a PR with one approval, but the specs and the tests do not fully agree with the Note that I am not believing in having a new feature has a separate project. It is already the case for the Finally, I believe that the best thing would be to have |
Personally I think 2 approvals is not the problematic point, but indeed finding the time for reviews, or ideally new reviewers. Maybe once a PR reaches a certain level of maturity, submitting a description and demonstration to Discourse could raise more interest.t I'm hesitant to make submitting a fpm package a requirement. It could be a recommendation, and I know @awvwgk has done this many times (also in issues), but it requires a lot of effort, and I fear would raise the bar too high for new contributors. |
@jvdp1 I share your concerns, and that's why I opened up this issue. I think you are saying not to merge too quickly to the experimental part of stdlib. You are also saying not to use separate fpm projects. As such, the third way forward that I think you are suggesting is to make it so that |
There is fortran-lang/fpm#78 open since a while, it will take a reasonable effort to integrate the preprocessing stage in the current target generation structure. It's a small to medium-sized project and will require some time commitment mind you, but the fpm maintainers are of course around to provide pointers if needed. If anyone here feels deeply about building stdlib with fpm without running the deploy script locally first, this is the right place to put in some energy and effort. |
@certik You summarized nicely my message. Thank you. However, I hope that compiling any branch with |
I'll keep trying to push some PRs/issues forward since I have time to spare now (but I don't know how long I can consistently do it or even if it's ok - please let me know).
I'm not sure if it's the case, but one thing that may help is to ask (encourage) everyone to do some light reviews even if you are not 100% familiar with the PR topic, the more people looking at the code the better (especially in huge PRs). |
After reading some of the comments here I feel that another pain point is "how easy is it to get someone's PR locally to actually test it and review it"? Which is not necessarily an easy task. Skimming around I found this post https://andrewlock.net/working-on-two-git-branches-at-once-with-git-worktree/ that addresses precisely this by showing how to use |
I didn't know
Maybe. However, I think that the best would be that |
Ok, so I think I understood where my confusion was, and I see that there are two issues:
Just slightly disconnected, https://fortran-lang.discourse.group/t/what-kind-of-tests-are-sufficient-some-personal-thoughts/7137/7 the video linked here is honesty a gem regarding building up the ecosystem, it is worth's listening to the whole talk :) |
One thing that I am worried is that we are making the same mistake as the standards committee, often standardizing things before we get actual experience with using it. Now with fpm, I wonder if it's time to first require creating an fpm package with the addition, and get people using it, before standardizing it in stdlib?
Take #580 as an example. It's been open for half a year, but how many people have actually tried it in their projects? I didn't, because it's not that easy to test a PR, you have to clone the repository, checkout the PR, install stdlib by hand, etc. You can't use it via fpm (as far as I know, since fpm requires a special branch, that is not generated from PRs), we could fix that by committing all the fypp generated files into git, then we could update
fpm
to support depending on PRs. However, even that seems relatively fragile and temporary.Instead, if #580 was posted in a standalone git repository as a regular package, then people can immediately use it in their projects using fpm. If it later gets standardized, then it's a simple change of an fpm dependency, and possibly of a
use
line. And there is no rush with this change either. People could contribute patches to the repository, tests, etc.An example would be the
argparse
package in the Python standard library, which first lived as a standalone package, well liked, and then later standardized.The text was updated successfully, but these errors were encountered: