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

Proposal: Move idna_adapter crate to servo/rust-url repository #1009

Open
Foorack opened this issue Dec 18, 2024 · 9 comments
Open

Proposal: Move idna_adapter crate to servo/rust-url repository #1009

Foorack opened this issue Dec 18, 2024 · 9 comments

Comments

@Foorack
Copy link

Foorack commented Dec 18, 2024

Currently, the idna_adapter crate is hosted under the "github.com/hsivonen" prefix.

Given that idna_adapter's sole publicly reported dependant is the idna crate, I propose moving idna_adapter source code to the servo/rust-url repository.

image

  • Centralization: Simplifies management and maintenance by centralizing related codebases.
  • Consistency: Ensures consistent development and versioning of interdependent crates.
  • Security: Enhances security by consolidating code under the main servo organization, reducing the risk of fragmentation.
  • Transparency: Improves transparency and trust by having all related code in the main servo repository.
@Manishearth
Copy link
Member

This was a deliberate and hopefully temporary choice while we were still figuring out exactly how to do this. I believe @hsivonen is open to moving this into the servo org or this repo based on what the servo TSC decides.

This repo seems fine by me.

@Manishearth
Copy link
Member

@hsivonen what was the blocker to doing this before? Just needing to wait for a TSC meeting?

@Sytten
Copy link

Sytten commented Dec 18, 2024

Came here to say the same thing, it feels very wrong for a crate that is now included in a lot of codebases via the url crate to be hosted on the personal account of a maintainer.

@hsivonen
Copy link
Collaborator

I wanted to move the repo from under github.com/hsivonen/ to under github.com/servo/ , but the Servo Technical Steering Committee was uneasy with taking more repos at this time. The idea of moving rust-url itself out of the servo GitHub org was floated by a TSC member. Settling that issue one way or another is probably in practice a prerequisite for moving the idna_adapter and idna_mapping repos.

Due to the way the testing and development mechanics of the idna_adapter crate go, it would be inconvenient to put the crate in the rust-url repo. I think idna_adapter should remain in a separate repo so that it's easy to switch branches in a checkout independently of the branch of the rust-url repo that has been checked out.

@Manishearth
Copy link
Member

Due to the way the testing and development mechanics of the idna_adapter crate go, it would be inconvenient to put the crate in the rust-url repo. I think idna_adapter should remain in a separate repo so that it's easy to switch branches in a checkout independently of the branch of the rust-url repo that has been checked out.

I don't think it would be hard to write such a test in-tree. I'm happy to take on that work if that is the only blocker.

@hsivonen
Copy link
Collaborator

hsivonen commented Dec 20, 2024

How do you mean? idna_adapter currently has 4 branches that are relevant to switch between while having the rust-url checkout in a single state: main, unicode-rs, no-unicode, and icu4x-trunk.

@Manishearth
Copy link
Member

@hsivonen we're not actually testing them that way in either repo.

What we are testing is 1.2.0 and 1.0.0. I don't see a problem with having two folders in tree here, idna-adapter and idna-adapter-1.x. The 1.x folder is excluded from the workspace but uses path dependencies for tests.

We could also maintain an idna-adapter-1.x branch on rust-url and merge rust-url code back into it on occasion. I think I'd prefer this.

I think there are a couple techniques available for this.

@nicoburns
Copy link

@hsivonen what was the blocker to doing this before? Just needing to wait for a TSC meeting?

While there was a little pushback from the Servo TSC on the issue of more repos, I think it was mostly a case of "we feel like this ought to be decided and managed by rust-url maintainers" (which according to MAINTAINERS.md is just @valenting (is there anyone else who ought to be on that list?)) and the TSC not wanting to subvert the "rust-url maintainers" authority for a request from someone (@hsivonen) who was unknown to them.

I think there was also a suggestion that idna-adapter could just live in this repo (the main rust-url repo) along with the main idna crate. But the issue was really more of an "authority vacuum" than any substantial concerns.

@hsivonen
Copy link
Collaborator

hsivonen commented Jan 3, 2025

We could also maintain an idna-adapter-1.x branch on rust-url and merge rust-url code back into it on occasion. I think I'd prefer this.

This kind of thing is what I have been trying to avoid.

Currently, the development process is changing idna_adapter dependency in a checkout of the rust-url repo to a path reference to an adjacent idna_adapter checkout and then switching between the branches of idna_adapter knowing that this does not change anything on the rust-url side.

In particular, this allows for in-flight rust-url changes to participate in local testing with multiple idna_adapter branches without having to copy the in-development rust-url change to multiple branches of rust-url for multiple flavors of idna_adapter.

The location of the tests isn't particularly nice at the moment:

idna_adapter 1.0.0 explicitly renders the idna crate non-compliant with UTS 46, so it obviously cannot be tested with a test harness that uses any version of the UTS 46 test suite. Tests for idna_adapter 1.0.0 are in https://github.com/hsivonen/test_no_idna/ .

For idna_adapter 1.2.x the supported UTS 46 version is determined by the ICU4X version. For idna_adapter 1.1.0 the supported UTS 46 version is determined by the idna_mapping dependency. Yet, the version of the UTS 46 test suite is determined by the rust-url repo.

It's currently the case that the version of the UTS 46 test suite isn't just a matter of versioning IdnaTestV2.txt, because the official test suite had a bug relative to the spec requirements, so the test harness needs adjustments. This was fixed in the Unicode 16.0 release of the UTS 46 test suite, so the currently in-flight changes involve changing the test harness together with IdnaTestV2.txt for Unicode 15.1 vs. Unicode 16.0.

Considering that the UTS 46 conformance test version isn't actually tied to the idna crate version but to crates a couple of steps deeper in the dependency tree, it's not particularly useful that we currently include the conformance test harness and upstream data file in the idna crate on crates.io. We should probably stop including the UTS 46 test data file and the associated harness in the crates.io release. (This would also avoid including test data that's under different license, Unicode-3.0, than the parts of the crate that you actually use when doing a normal build.)

I'd find it useful to move the UTS 46 conformance test, too, outside the rust-url repo so as to be able to switch branches independently of the rust-url repo.

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

No branches or pull requests

5 participants