Skip to content

Conversation

@elldritch
Copy link
Contributor

This PR modifies json-schemabin/src/main.rs so that the script attempts to fs::copy if the fs::rename fails. On my machine, this fails because I'm running Arch Linux and /tmp is mounted on a different device than /home.

You can see another example of this error in rust-lang/rustup#1239.

Ideally, we would match on e.kind(), but unfortunately the CrossesDevices error enum value is not yet available in stable, only nightly (see https://doc.rust-lang.org/stable/std/io/enum.ErrorKind.html#variant.CrossesDevices).

Another simpler alternative here would be to always do a copy instead of a rename, although this might slow down builds a little bit on unaffected machines.

Copy link
Owner

@rbtying rbtying left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this!

The types update is rarely run, so I think always using copy is fine from a performance perspective. Do you mind doing that?

Thanks!

@elldritch elldritch force-pushed the elldritch/fix/yarn-types-linux-cross-device-rename branch from df65335 to 4934ae1 Compare December 2, 2024 23:40
@elldritch
Copy link
Contributor Author

Done. I've also rebased on to latest master.

The diff also contains changes to the generated files (from when I ran yarn types), but I don't think any of those changes are semantic. Here's what difftastic says about the AST diff:

➜ git --no-pager -c diff.external='difft --display inline' diff HEAD^
frontend/json-schema-bin/src/main.rs --- Rust
55                return;
56            }
57        }
58
59        - std::fs::rename(&tmp_path, path).unwrap()
   59     + std::fs::copy(&tmp_path, path).unwrap();
   60 }

frontend/src/gen-types.d.ts --- 1/4 --- TypeScript
160          PlayCardsWithHint: [Card[], TrickUnit[]];
161        };
162    export type Number = string;
163    export type FriendSelectionPolicy =
164    -   | "Unrestricted"
171    -   | "JokerOrHigherSuit"
179    -   | "BothTwoOrMore"
   163 + export type FriendSelectionPolicy = "Unrestricted" | "TrumpsIncluded" | "HighestCardNotAllowed" | "PointCardNotAllowed";
   166 + export type BidPolicy = "JokerOrHigherSuit" | "JokerOrGreaterLength" | "GreaterLength";
   171 + export type JokerBidPolicy = "BothTwoOrMore" | "BothNumDecks" | "LJNumDecksHJNumDecksLessOne" | "Disabled";
   172 export type MaxRank = string;

frontend/src/gen-types.d.ts --- 2/4 --- TypeScript
189            [k: string]: unknown;
190          };
191        };
192    export type AdvancementPolicy =
193    -   | "Unrestricted"
197    -   | "NoBonusLevel"
   181 + export type AdvancementPolicy = "Unrestricted" | "FullyUnrestricted" | "DefendPoints";
   182 + export type BonusLevelPolicy = "NoBonusLevel" | "BonusLevelForSmallerLandlordTeam";
   183 export type KittyPenalty = "Times" | "Power";
   184 export type KittyBidPolicy = "FirstCard" | "FirstCardOfLevelOrHighest";
   185 export type TrickDrawPolicy =

frontend/src/gen-types.d.ts --- 3/4 --- TypeScript
242          };
243        };
244    export type Suit = string;
245    export type EffectiveSuit =
246    -   | "Unknown"
   229 + export type EffectiveSuit = "Unknown" | "Clubs" | "Diamonds" | "Spades" | "Hearts" | "Trump";

frontend/src/gen-types.d.ts --- 4/4 --- TypeScript
724       * A parallel array to `played_cards` which contains the units corresponding to played cards that match the `trick_format`, or `None` if they don't match.
725       *
726       * TODO: remove default deserialization attribute in a few days.
727       */
728    - played_card_mappings?: Array<TrickUnit[] | null>;
   706 + played_card_mappings?: (TrickUnit[] | null)[];
   707   played_cards: PlayedCards[];
   708   player_queue: number[];
   709   trick_format?: TrickFormat | null;
   710   trump: Trump;

frontend/src/gen-types.schema.json --- JSON
No syntactic changes.

It removed some leading |s from sum types, and changed an Array<T> to a T[].

@rbtying
Copy link
Owner

rbtying commented Dec 3, 2024

@elldritch I think you might be missing a yarn prettier --write

@elldritch
Copy link
Contributor Author

elldritch commented Dec 3, 2024

@rbtying Good catch! Pushed.

EDIT: It looks like this still chose to prefer T[] over Array<T>. Not sure why, given that the prettier version in the lockfile hasn't been updated since 2022 and the last regeneration of this file was 2024. Here's the version my system is using:

➜ yarn prettier --version
yarn run v1.22.22
$ prettier src --version
2.5.1
Done in 0.63s.

➜ node --version
v23.1.0

@rbtying
Copy link
Owner

rbtying commented Dec 3, 2024

Hm, perhaps we should update prettier here since it fails eslint now!

/home/runner/work/shengji/shengji/frontend/src/gen-types.d.ts
  728:26  error  Array type using 'T[]' is forbidden for non-simple types. Use 'Array<T>' instead  @typescript-eslint/array-type

Let me take a stab at this later, though -- thank you for getting it this far!

@rbtying rbtying merged commit 9d378fc into rbtying:master Dec 19, 2024
2 of 3 checks passed
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

Successfully merging this pull request may close these issues.

2 participants