NixPath -> NixString and Error -> Errno

[arcnmx]

Major API change, it passes on the responsibility of dealing with CString conversions to the caller. The error type is now just Errno itself.

Closes #221

cc @kamalmarhubi

[kamalmarhubi]

Is the idea to drop NixString once your impl AsRef<CStr> for CString lands?

[kamalmarhubi]

Did @carllerche ever comment on #221?

[arcnmx]

Is the idea to drop NixString once your impl AsRef<CStr> for CString lands?

That's correct, and why I named the method as_ref(). I should add a comment describing that. It will land in 1.7.0. The whole NixPath thing is still waiting on comments from @carllerche, but I needed to make these changes for my own project so I went ahead anyway >.>

[carllerche]

Sorry for the delay! I'm commenting on #221

[kamalmarhubi]

Doing some PR triage. This one's going to hang around a bit longer pending #221, I think!

[arcnmx]

Hmmm, now this PR fails because ToOwned wasn't implemented for CStr until rust 1.3.0... Awkward :(

Might have to use a build script to determine version features? I wonder if that would help for also replacing NixString with AsRef<CStr> for 1.7.0+...

[kamalmarhubi]

Hmmm, now this PR fails because ToOwned wasn't implemented for CStr until rust 1.3.0... Awkward :(

Well #238 hasn't been closed yet... we could discuss a bit in there? There's also some discussion on the users discourse instance which I'll link to on the other issue.

Might have to use a build script to determine version features? I wonder if that would help for also replacing NixString with AsRef<CStr> for 1.7.0+...

Not sure what you mean there?

[arcnmx]

Not sure what you mean there?

We could add some conditional compilation magic into nix_string.rs:

1. If the compiler is older than 1.3.0, don't impl NixString on Cow<CStr>
2. If the compiler is 1.7.0 or newer, don't use NixString at all. Simply define it as pub type NixString = AsRef<CStr>;

This is what the rustc_version crate is for.

[kamalmarhubi]

I was thinking about how to get further along with what this PR is trying to do. Let's break it down into a few stages:

• make a PR introducing Errno as a new type, along with its impls, adding an impl From<NixError> for Errno and an impl From<Errno> for NixError for interoperability
• make a PR introducing NixString along with its impls
• then we can do a migration over a series of PRs, say one per module
• remove the old NixError and NixPath when that's all done

I think we can get more people to weigh in on the changes if the PRs are smaller. We can also have more focused discussion of two individual changes.

I also think it'll help because right now pretty much every merged PR will cause conflicts with this one, making it hard to land.

[kamalmarhubi]

I'm happy to help break it up if you like, just let me know!

[arcnmx]

maybe use the feature name so std_has_cstr_borrow:

mm sorry was just testing on CI. It's probably something that should go in its own PR anyway.

make a PR introducing Errno as a new type, along with its impls, adding an impl From<NixError> for Errno and an impl From<Errno> for NixError for interoperability

Errno is already a type, this PR doesn't introduce it. It only removes the Error type because it existed to differentiate errors between Errno and InvalidPath, the latter of which no longer exists due to NixString.

However, we can land Errno::result() first, just as a cleanup that moves the whole repo to the new from_ffi.

• make a PR introducing NixString along with its impls
• then we can do a migration over a series of PRs, say one per module
• remove the old NixError and NixPath when that's all done

This is a strange workflow to me since the intermediate state of the repo would be useless. I guess if we want to break it up I'd do it more like...

• Whitespace fixes
• Errno:result()
• NixString and impls
• Migrate all methods to NixString.
• Remove NixError

The last three seem like a single PR to me, breaking it up by module doesn't seem particularly productive as it's just a search-replace NixPath -> NixString. The feedback on that change won't be limited to any particular function or module, it's a fundamental change that raises the question: do we want to remove the stack allocation, copy, and path length check (thus delegating CString conversions to the caller) or not? Landing NixString impls before finalizing that discussion is premature... but once the course of action is chosen there's nothing left to discuss!

[arcnmx]

So as it turns out, #247 makes up the bulk of the changes. c43753350cb7b1465a56858fc4ee4b2bdaab1c31 is the change on top of that.

[kamalmarhubi]

@arcnmx since #247 (finally!) got merged, could you drop those commits from this PR and change its title?

[arcnmx]

Rebased. The title still applies.

[arcnmx]

Note that the cstr module is mostly for concenience to ease the transition, and used in the tests for that purpose. I'd suggest maybe pulling it out as a separate crate rather than having it built into nix proper.

[homu]

:umbrella: The latest upstream changes (presumably #271) made this pull request unmergeable. Please resolve the merge conflicts.

[Susurrus]

Considering how old this is and how it looks like at least some of this was merged in #247, is there anything worth salvaging from this PR and someone willing to push this work forward? If not, let's go ahead and close this.

[arcnmx]

The PR still applies, although I'm not sure whether any conclusions were come to re #221. I'm happy to help update/rebase if a particular design is agreed on as the best way forward. This particular PR makes every API call just AsRef<CStr>, but there are other options/alternatives as discussed in that issue.