georust / topojson

TopoJSON bindings and utilities for Rust
MIT License
10 stars 8 forks source link

Migrate to the 2021 edition #19

Closed yozhgoor closed 2 years ago

yozhgoor commented 2 years ago

Hello there!

This PR migrate the project to the 2021 edition using cargo fix --edition

yozhgoor commented 2 years ago

I wanted to fix an error message when I try to migrate from the 2018 edition to the 2021 in a project but I don't see it on the fork

The error message look like this:

error[E0391]: cycle detected when computing when `topojson::geometry::Value` has a significant destructor
  --> /home/user/.cargo/registry/src/github.com-1ecc6299db9ec823/topojson-0.5.0/src/geometry.rs:21:1
   |
21 | pub enum Value {
   | ^^^^^^^^^^^^^^
   |
   = note: ...which immediately requires computing when `topojson::geometry::Value` has a significant destructor again
note: cycle used when computing when `topojson::geometry::Geometry` has a significant destructor
  --> /home/user/.cargo/registry/src/github.com-1ecc6299db9ec823/topojson-0.5.0/src/geometry.rs:87:1
   |
87 | pub struct Geometry {
   | ^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0391`.
yozhgoor commented 2 years ago

Cool! :rocket: What do you think about the error message? Do you mind if I open an issue about that to merge this PR?

lnicola commented 2 years ago

I've never seen that error before and I didn't really get the specifics. Are you saying that:

My understanding is that dependencies should keep working when you change editions. If I got it right, can you put together a simple test case? I tried to reproduce it with an empty project with the topojson dependency, but it builds fine. Which compiler version are you using?

yozhgoor commented 2 years ago

rustc 1.60.0 at the moment. You get it right, I also tried on a empty project and no problem too. I'm gonna investigate the issue on my side to see if I can find something useful. It probably comes from my implementation. I'll keep you update in that case, thanks for your time!

lnicola commented 2 years ago

This is a small change, can someone from the project review and/or merge it?

I think I have bors rights, but I've only contributed to other parts of GeoRust and I don't know the maintainers here.

lnicola commented 2 years ago

In the meanwhile, it might even make sense to go to the 2021 edition. Some people would prefer to bump the version to 0.6 here, but it's not clear-cut. Personally, I don't think MSRV changes are semver-breaking.

lnicola commented 2 years ago

bors r=pjsier,lnicola,urschrei

bors[bot] commented 2 years ago

:lock: Permission denied

Existing reviewers: click here to make lnicola a reviewer

urschrei commented 2 years ago

I don't have access to bors on this repo for some reason

michaelkirk commented 2 years ago

@frewsxcv seems to have bors access. Can you add some other folks?

(https://github.com/georust/topojson/pull/6#issuecomment-439750356)

frewsxcv commented 2 years ago
Screen Shot 2022-06-16 at 2 40 53 PM

done

michaelkirk commented 2 years ago

bors r=pjsier,lnicola,urschrei

bors[bot] commented 2 years ago

Configuration problem: bors.toml: not found

urschrei commented 2 years ago

bors try

urschrei commented 2 years ago

bors r=pjsier,lnicola,urschrei

bors[bot] commented 2 years ago

Timed out.

lnicola commented 2 years ago

I think we need different workflow job names in bors.toml:

image

urschrei commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Already running a review

urschrei commented 2 years ago

bors r-

urschrei commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Already running a review

urschrei commented 2 years ago

¯_(ツ)_/¯

lnicola commented 2 years ago

See https://github.com/georust/topojson/blob/master/bors.toml.

status needs to list the job names that bors was to wait for. The CI here uses different job names, not ci result.

urschrei commented 2 years ago

I just tried to change that: https://github.com/georust/topojson/blob/master/.github/workflows/test.yml#L20

lnicola commented 2 years ago

bors retry

bors[bot] commented 2 years ago

try

Already running a review

lnicola commented 2 years ago

bors r-

cecton commented 2 years ago
error[E0391]: cycle detected when computing when `topojson::geometry::Value` has a significant destructor
  --> /home/user/.cargo/registry/src/github.com-1ecc6299db9ec823/topojson-0.5.0/src/geometry.rs:21:1
   |
21 | pub enum Value {
   | ^^^^^^^^^^^^^^
   |
   = note: ...which immediately requires computing when `topojson::geometry::Value` has a significant destructor again
note: cycle used when computing when `topojson::geometry::Geometry` has a significant destructor
  --> /home/user/.cargo/registry/src/github.com-1ecc6299db9ec823/topojson-0.5.0/src/geometry.rs:87:1
   |
87 | pub struct Geometry {
   | ^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0391`.

I had this issue too when I migrated to 2021 on a private project... I think (not sure) I solved it by adding Box on types of fields that was recursive.

So let's say I have an enum A that is recursive (some variants have Box<A> in their fields). Then I have a struct B that uses the enum A in a field. I added Box on the field in struct B to fix the issue struct B { field_a: Box<A> }.

I'm really not certain about this, it's been a long time.

urschrei commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Already running a review

urschrei commented 2 years ago

bors try

bors[bot] commented 2 years ago

try

Already running a review

urschrei commented 2 years ago

I have no idea why bors is being like this, but I'm happy to manually merge this now that CI has passed, in the interest of moving forward. @cecton thanks for the info: there doesn't seem to be an issue on current stable – it may have been transient.

bors[bot] commented 2 years ago

try

Timed out.

cecton commented 2 years ago

I have no idea why bors is being like this,

clearly he's bored, you're not treating it well

@cecton thanks for the info: there doesn't seem to be an issue on current stable – it may have been transient.

all the work i did for nothing :weary: (good thing I don't remember)