license should be from relative dir

[gilescope]

As mentioned in #3352, if the license file is set and we're not in the root dir when calling cargo fmt it currently falls in a heap.

[gilescope]

@calebcartwright I can attest to this feature being pretty broken at the moment so this should be an easy review - it can't make the feature any worse.

[gilescope]

Can I help in any way or do anything more?

[calebcartwright]

Can I help in any way or do anything more?

Not really. I appreciate the PR and that folks would probably prefer I get through issue and PR triage a bit more quickly 😄

However, the amount of work greatly overshadows the amount of bandwidth so the pace is what circumstances permit. I realize that this seems like a small/easy change, but path resolution changes have been a notorious source of bugs, often fixing one issue while creating another (especially on different platforms). It's not an area we have great tests for which ups the need for review/manual testing, and to be fully transparent, some of what I'll call rustfmt's auxiliary features (license checks, todo scans, etc.) are simply lower on the priority list relative to more core formatting issues.

Hmm but then I see dunce #4137 and then at some point it is removed... - trying to figure out why.

Probably best summarized in #3914 (comment), but that's part of the story in the review. There's been lots of various changes around path resolution (both for licenses/configs as well as loading out-of-line mods during the earlier parsing phases) made on the other branch that I'll need to cross reference.

Sorry I don't have any better news, but promise I'll get to it when I can!

[calebcartwright]

@karyon - realize you're still in the early discovery phase but as you're reviewing the various non-backported changes from the 2.0 branch, this is one area I'd like to prioritize when time permits.

I think there were a few fixes as well as a different implementation that utilized some different dependencies. I'd love to get a better feel for if/how we should best backport those so that we can either get the underlying issues resolved or at least know whether we'd be setting up some gnarly merge conflicts if we were to press forward with a change like this prior to any backporting

[karyon]

@calebcartwright as far as I can see, there have been not many changes to config_type.rs or license.rs in the 2.0 branch, and none of them were large or looked relevant to me. So I think technically there shouldn't be major merge conflicts when backporting stuff. But I can't tell how much the backported features will indirectly interfere with this PR here :(

I searched for backportable path resolution PRs with this query. Here are the PRs that looked potentially relevant:

• switch to non-recursive mode by default #3938
• Use absolute_path instead of fs::canonicalize #4131 ( replaced by the next one)
• Use duncet to fix path issues #4137
• Fix module resolution in inner modules with paths #4194
  I can try and backport those if you want, although I'd probably need a lot of help with the first one.

Does that help? Let me know what I should do!

[calebcartwright]

Thanks @karyon, though none of those were what I was thinking of for this. I thought we had some other PRs that dealt with addressing config file resolutions in cases where there were multiple config files throughout the project. The dunce one is worth pursuing, and the others relate to some other areas that need attention, but I don't think of any of those will be applicable here.

If we are to go forward with this fix/enhancement to the license feature, I'd want to see some thorough test cases that cover the behavior. That should include scenarios with multiple config files with respective templates, as well as file-based definitions with command line overrides.

That being said, this is another config option, like report_todo and report_fixme, that just feels out of place for rustfmt. I understand the utility of the license check feature (and the other two as well), but I keep coming back to the conclusion those don't belong in rustfmt/would be better suited as a standalone tool.

[gilescope]

Just to be clear the licensing feature is pretty broken at the moment.

…

On Mon, 1 Nov 2021 at 03:06, Caleb Cartwright ***@***.***> wrote: Thanks @karyon <https://github.com/karyon>, though none of those were what I was thinking of for this. I thought we had some other PRs that dealt with addressing config file resolutions in cases where there were multiple config files throughout the project. The dunce one is worth pursuing, and the others relate to some other areas that need attention, but I don't think of any of those will be applicable here. If we are to go forward with this fix/enhancement to the license feature, I'd want to see some thorough test cases that cover the behavior. That should include scenarios with multiple config files with respective templates, as well as file-based definitions with command line overrides. That being said, this is another config option, like report_todo and report_fixme, that just feels out of place for rustfmt. I understand the utility of the license check feature (and the other two as well), but I keep coming back to the conclusion those don't belong in rustfmt/would be better suited as a standalone tool. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4967 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGEJCBQ5Y7LTPAMKGUD2HDUJX73ZANCNFSM5DASFAUQ> .

[calebcartwright]

Just to be clear the licensing feature is pretty broken at the moment.

Thanks @gilescope, though to be honest I'm really not sure what to make of this given your prior comment to this same effect and the subsequent conversation.

To put it a little more pointedly, I'm seriously considering moving to drop this feature from rustfmt altogether, along with some of the other non-formatting related features that were stuffed into rustfmt for some reason.

However, if this is a feature we're going to keep, and if we're going to move forward with your proposed changes, then it would need to have some accompanying test cases to cover the various scenarios I described in #4967 (comment). I'm not requesting those tests at the moment though, as I wouldn't want you or anyone else to work on them in futility given the possible dropping/deprecation of this unstable feature.

[gilescope]

Agreed, do or do not :-) Idk maybe it should live in cargo deny conceptually?

…

On Sat, 6 Nov 2021 at 00:28, Caleb Cartwright ***@***.***> wrote: Just to be clear the licensing feature is pretty broken at the moment. Thanks @gilescope <https://github.com/gilescope>, though to be honest I'm really not sure what to make of this given your prior comment to this same effect <#4967 (comment)> and the subsequent conversation. To put it a little more pointedly, I'm seriously considering moving to drop this feature from rustfmt altogether, along with some of the other non-formatting related features that were stuffed into rustfmt for some reason. However, if this is a feature we're going to keep, and if we're going to move forward with your proposed changes, then it would need to have some accompanying test cases to cover the various scenarios I described in #4967 (comment) <#4967 (comment)>. I'm not requesting those tests at the moment though, as I wouldn't want you or anyone else to work on them in futility given the possible dropping/deprecation of this unstable feature. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4967 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGEJCG67PESGOHPDNSJNO3UKSAD5ANCNFSM5DASFAUQ> .

[calebcartwright]

Thanks again for the PR and bearing with us while we contemplated the future of the option. I've decided we're going to move forward with the deprecation path so I'm going to close this.

Refs #5103