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

fix afsctool for High Sierra #20898

Closed
wants to merge 1 commit into from

Conversation

heavyk
Copy link
Contributor

@heavyk heavyk commented Nov 20, 2017

patch file type to allow file type 24 as well
ref: diimdeep/afsctool#3
original diff: https://github.com/gingerbeardman/afsctool/commit/1fab0d87546f36e24e89a9f73f187b3ff639b920.diff?full_index=1

modified path in patch to afsctool_34/afsctool.c

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

patch file type to allow file type 24 as well
ref: diimdeep/afsctool#3
original diff: https://github.com/gingerbeardman/afsctool/commit/1fab0d87546f36e24e89a9f73f187b3ff639b920.diff?full_index=1

modified path in patch to afsctool_34/afsctool.c
@fxcoudert
Copy link
Member

It doesn't work natively on 10.12 and 10.13, there has been no sign since 2014 that the original author intends to maintain it.

@ilovezfs ilovezfs added the 10.13 High Sierra is specifically affected label Nov 21, 2017
@ilovezfs
Copy link
Contributor

@brkirch can we get this upstreamed in a new release?

@gingerbeardman
Copy link
Contributor

gingerbeardman commented Nov 25, 2017

With my patch (in OP) it works on 10.12 Sierra and 10.13 High Sierra

there's also this currently maintained version: https://github.com/RJVB/afsctool

@ilovezfs
Copy link
Contributor

@gingerbeardman either @brkirch would need to bless one of the forks, or a fork would need its own project name and need to meet the brew audit --new-formula notability requirements.

@heavyk
Copy link
Contributor Author

heavyk commented Nov 27, 2017

@gingerbeardman that rjvb fork is really nice! I'll update the formula to use that version soon. it's clearly superior to your branch and also looks to be regularly maintained

@heavyk
Copy link
Contributor Author

heavyk commented Nov 27, 2017

I still have yet to try the @RJVB version out, however, I did notice the f_type is not even checked. I'm assuming that file compression is attempted and then when errors occur, the changes are rolled back.

EDIT: turns out the f_type check is changed to:

(!strncasecmp(fsInfo.f_fstypename, "hfs", 3) || !strncasecmp(fsInfo.f_fstypename, "apfs", 4))

in this commit: RJVB/afsctool@4d3272d
discussion: RJVB/afsctool#1

@ilovezfs
Copy link
Contributor

Closing this out until we hear from upstream. Thanks @heavyk

@ilovezfs ilovezfs closed this Nov 27, 2017
@heavyk
Copy link
Contributor Author

heavyk commented Nov 27, 2017

ok I updated the formula here if you want to pull it in:

heavyk@5b5d8af

@gingerbeardman
Copy link
Contributor

gingerbeardman commented Nov 27, 2017

Great. I only found the RJVB version after seeing this issue. So thanks! Much better than my old fork.

@gingerbeardman
Copy link
Contributor

gingerbeardman commented Apr 9, 2018

Just coming back to this. I will try to contact @brkirch again

How exactly will they need to "bless" it? Given that he keeps a low profile these days @ilovezfs @heavyk

@gingerbeardman
Copy link
Contributor

Thanks, I had already sent him a message. Though I doubt we will get a response. Fingers crossed 🤞

@heavyk
Copy link
Contributor Author

heavyk commented Apr 12, 2018

I've been using the RJVB version here locally for the last 4 months just fine... I hope that's the one that makes it into brew finally

@ilovezfs
Copy link
Contributor

@gingerbeardman
Copy link
Contributor

gingerbeardman commented Apr 12, 2018

@ilovezfs that's a nice alternative, thanks.

With regards to the "dead" formula, it's kind of worrying this can happen. For how long do such formula stick around before being retired? What happens if an author dies or ceases being interested?

I also think it's odd that the author's permission was not sought for the formula to be added, but it is needed for this worthwhile change that will benefit all. Had the initial permission been sought you'd have realised the author is totally incommunicado.

@ilovezfs
Copy link
Contributor

Formulae are typically retained until they break and fixes cannot be upstreamed. If an upstream becomes completely unavailable, then a fork may be able to take its place depending on circumstances. In this case, based on the responses on RJVB/afsctool#1 I don't have the requisite confidence in that fork.

@gingerbeardman
Copy link
Contributor

gingerbeardman commented Apr 12, 2018

Ah, all is now clear - you're holding a grudge. You've not got your way so you're creating obstacles.

Hopefully we can get somebody else involved who doesn't have a vendetta.

Any thoughts @MikeMcQuaid @fxcoudert @JCount @mistydemeo @tschoonj ?

@ilovezfs
Copy link
Contributor

@gingerbeardman please read or re-read our Code of Conduct and adjust your future communications accordingly.

@gingerbeardman
Copy link
Contributor

I've read it, understood.

Any thoughts on my points above?

Copy link

@brkirch brkirch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay, this can be merged in.

@ilovezfs ilovezfs reopened this Apr 12, 2018
@ilovezfs
Copy link
Contributor

@brkirch if you could post a 10.13 compatible version, that would be great.

@Homebrew Homebrew deleted a comment from RJVB Apr 12, 2018
@ilovezfs ilovezfs added build failure CI fails while building the software audit failure CI fails while auditing the software labels Apr 12, 2018
@brkirch
Copy link

brkirch commented Apr 12, 2018

Any fork with major changes can have a formula submitted under a different program name, and I don't mind at all if "afsctool" is included in the name so long as it isn't the full name.

Originally I created afsctool just as a way to identify compressed files and accurately determine the disk space such files used. The other features were quite frankly an afterthought and as such what afsctool really needs is a complete rewrite. I started on such a rewrite a long while back but have never completed it. Even though it has been a long time I still wish to finish it eventually.

@ilovezfs Although I don't know how much time I'll have to spend on it, I'll see if I can look into that within the next few days.

@ilovezfs
Copy link
Contributor

Thanks, @brkirch. That would be much appreciated.

@ilovezfs
Copy link
Contributor

ilovezfs commented Apr 12, 2018

@ilovezfs Can commit 5b5d8af be reverted? The commit I approved was 6bd9cd2.

@brkirch done.

@ilovezfs ilovezfs removed audit failure CI fails while auditing the software build failure CI fails while building the software labels Apr 12, 2018
@brkirch
Copy link

brkirch commented Apr 12, 2018

@ilovezfs Thanks, I actually didn't notice the other commit there when I was looking before.

@heavyk The enhancements in https://github.com/RJVB/afsctool do look nice so if @RJVB doesn't mind then it doesn't seem like a bad idea to submit a formula for it under a different name. Like I said in my above comment, it would be fine if "afsctool" is included in the name (e.g. afsctool_rjvb).

@gingerbeardman
Copy link
Contributor

@brkirch the parallel processing fork is actually by @RJVB

@ilovezfs ilovezfs closed this in 77810f6 Apr 12, 2018
@ilovezfs
Copy link
Contributor

Thanks for your first contribution to Homebrew, @heavyk! Without people like you submitting PRs we couldn't run this project. You rock!

@brkirch
Copy link

brkirch commented Apr 12, 2018

@gingerbeardman Thanks for pointing that out, I've corrected my comment.

@ilovezfs
Copy link
Contributor

@brkirch for now, I've shipped this as-is with the patch inlined in the formula, but we look forward to a new upstream release tarball when you have a chance so that we can remove the patches. In terms of enhancements from the fork(s), my suggestion would be to have a https://github.com/brkirch/afsctool repository on GitHub so that people can open pull requests and get those upstreamed into your version if you'll accept them. The forks are unlikely to qualify on their own as new formulae in terms of the notability threshhold. Of course, as I mentioned above, they could also offer the alternative versions in their own tap.

@MikeMcQuaid
Copy link
Member

Ah, all is now clear - you're holding a grudge. You've not got your way so you're creating obstacles.

Hopefully we can get somebody else involved who doesn't have a vendetta.

Any thoughts @MikeMcQuaid @fxcoudert @JCount @mistydemeo @tschoonj ?

@gingerbeardman My thoughts are the following:

  • You made unfair accusations of @ilovezfs here when he's been polite and completely consistent with our existing policies
  • You attempted to escalate it because you didn't get your own way
  • When @brkirch did enter the discussion they said this:

Any fork with major changes can have a formula submitted under a different program name, and I don't mind at all if "afsctool" is included in the name so long as it isn't the full name.

This is exactly why this policy exists (and validates @ilovezfs's decision). We're not willing to have either major patches or move formulae to point to forks when either of them may have major issues which unfairly damage the reputation of the original author/software.

In future, @gingerbeardman, if you don't like or understand our policies I suggest asking for explanations and creating your own tap rather than criticising how we run this project. With no disrespect intended: unless you've run your own package manager for many years it's unlikely you'll have encountered or fully thought through all the edge cases we've had to.

That all said: thanks for your contributions to Homebrew and hopefully this can be seen as a minor request for course correction as we'd love you to continue to contribute in future.

@gingerbeardman
Copy link
Contributor

gingerbeardman commented Apr 12, 2018

That's all fair enough, thanks for the reply.

In hindsight I should have read the notability requirements when they were initially mentioned. Lesson learned.

I've only ever had one other unpleasant experience across all of the years and OSS projects I have participated in on GitHub, so I hope you'll understand that I feel I must stand my ground on this.

unfair accusation
Here is my thinking:

Formulae are typically retained until they break and fixes cannot be upstreamed. If an upstream becomes completely unavailable, then a fork may be able to take its place depending on circumstances.

"Sometimes we can make an exception"

In this case, based on the responses on RJVB/afsctool#1 I don't have the requisite confidence in that fork.

"But the fork owner didn't do what I wanted, the way I sugggested, so there's no chance"

That sort of snide remark is petty, seems arrogant, and is not really in the spirit of OSS I have come to expect. I wanted to call that out for the greater good. I also see that a comment by the fork owner was deleted (before I had a chance to read it, no idea what it said). I took it as validation of my initial remark.

So: accusation - yes; unfair - depends on your point of view.

escalation
I hoped that escalation would result in further discussion, which it has. I'm not the owner of the fork in question, so there's no "my way" for me to get or not.

brkirch decision
I'm happy that we've finally got a response, regardless of the outcome. The clarification about the fork renaming is absolutely fine by me - it's a resolution and that's exactly what this needed. The bonus is that we also got clarification that brkirch is still alive and interested in working on afsctool - far more than we've had for the past eight or so years.

@MikeMcQuaid
Copy link
Member

That sort of snide remark is petty, seems arrogant, and is not really in the spirit of OSS I have come to expect.

@gingerbeardman I'm going to assume good intentions here and hopefully explain to you why this is unacceptable. At this point you have accused @ilovezfs of the following:

  • holding a grudge
  • creating obstacles
  • [has] a vendetta
  • [being] snide
  • [being] petty
  • seem[ing] arrogant

That's simply not an acceptable way to speak to anyone on our issue tracker. It's a violation of our Code of Conduct (which you've been warned about before) and I'm formally warning you about again. Please keep your discussions technical and completely eliminate the name calling. Personally, I think it would also be polite to apologise to @ilovezfs but that's up to you.

To be clear on all of the above (with the exception of the recommendation to apologise): this is not a negotiation or discussion. I'm telling you your behaviour is unacceptable and you need to either adjust it or you will no longer be welcome in our community. This pains me to have to point out because you've previously contributed to Homebrew (which we genuinely appreciate) but I will sooner lose your future contributions than see you continue to speak to our about @ilovezfs in this way.

@gingerbeardman
Copy link
Contributor

gingerbeardman commented Apr 12, 2018

Consider my future behaviour adjusted and in accordance with the Code of Conduct. 🕊

I'd be happy to apologise to @ilovezfs if he is happy to apologise to @RJVB for the remark which resulted in my unacceptable behaviour? 🤝

In this case, based on the responses on RJVB/afsctool#1 I don't have the requisite confidence in that fork.

That said, I look forward to contributing further to homebrew in future. 🍻

@MikeMcQuaid
Copy link
Member

I'd be happy to apologise to @ilovezfs if he is happy to apologise to @RJVB for the remark which resulted in my unacceptable behaviour? 🤝

@gingerbeardman A conditional apology is not an apology so just to clarify for your own benefit: you're not willing to apologise. That's fine but I just think it's worth being explicit about.

Consider my future behaviour adjusted and in accordance with the Code of Conduct. 🕊
That said, I look forward to contributing further to homebrew in future. 🍻

Thanks ❤️

@gingerbeardman
Copy link
Contributor

There was nothing negative in my previous reply, so please don't imply that there was.

I did not mean to offer a conditional apology, I just meant that I feel there is more than one person here that is due an apology and neither of them are me. So let me take a leaf out of your book @MikeMcQuaid:

To @ilovezfs - I am sorry for any offence you may have taken. Personally, I think it would also be polite to apologise to @RJVB but that's up to you.

@MikeMcQuaid
Copy link
Member

@gingerbeardman Thank you ❤️

@lock lock bot added the outdated PR was locked due to age label May 12, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
10.13 High Sierra is specifically affected outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants