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.

Sign up for GitHub

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

Jump to bottom

arrow2-convert migration 3: attr.rust.override_crate & optimizing out dependencies #3897

Merged
merged 14 commits into from
Oct 18, 2023
Merged

arrow2-convert migration 3: attr.rust.override_crate & optimizing out dependencies #3897

merged 14 commits into from
Oct 18, 2023

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Oct 17, 2023 •
edited
Loading

This introduces the rust.attr.override_crate attribute, which is necessary for the serde-codegen story.

This attribute also allows us to generate some fundamental types such as InstanceKey directly into re_types_core rather than re_types, making it possible to reduce the number of crates that depend on the giant re_types.

  • The AutoSpaceViews & PanelView components are now back into their respective home (re_viewport & re_viewer, respectively).
    They were temporarily moved it because we had no support for custom_crate.
    They will be joined by their more complicated siblings in the next PR, which implements the necessary serde-codegen support.

  • InstanceKey, Clear and ClearIsRecursive are now generated into re_types_core.
    This allows re_query, re_arrow_store and re_data_store to drop their dependency on re_types.

  • Generated code now picks up arrow from re_types_core::external instead of importing it directly.
    This means crates hosting generated code are not forced into import arrow2 directly.

arrow2-convert migration PR series:

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested demo.rerun.io (if applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@teh-cmc teh-cmc added do-not-merge Do not merge this PR dependencies concerning crates, pip packages etc codegen/idl labels Oct 17, 2023
@teh-cmc teh-cmc force-pushed the cmc/generator_traits branch from 6443539 to 136f851 Compare October 17, 2023 13:15
@teh-cmc teh-cmc force-pushed the cmc/arrow2convert_be_gone_3_for_real branch from 0880035 to c029908 Compare October 17, 2023 13:18
Base automatically changed from cmc/generator_traits to main October 17, 2023 13:28
@teh-cmc teh-cmc force-pushed the cmc/arrow2convert_be_gone_3_for_real branch 2 times, most recently from 2081ac7 to 507b862 Compare October 17, 2023 14:51
@teh-cmc teh-cmc removed the do-not-merge Do not merge this PR label Oct 17, 2023
@teh-cmc teh-cmc mentioned this pull request Oct 17, 2023
Merged
4 tasks
@teh-cmc teh-cmc force-pushed the cmc/arrow2convert_be_gone_3_for_real branch from 507b862 to bb4da17 Compare October 18, 2023 06:43
@teh-cmc
Copy link
Member Author

teh-cmc commented Oct 18, 2023 •
edited
Loading

I have no clue why the C++ formatting check fails, I'm literally calling just codegen (and the file clearly looks formatted?!)... I've rebased and everything, we'll see how that goes this time 🤷

@emilk emilk self-requested a review October 18, 2023 08:17
emilk
emilk reviewed Oct 18, 2023
View reviewed changes
Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

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

image

aren't these files marked as generated anymore? GitHub doesn't show the usual text as it does for other things:

Uploading image.png…

crates/re_types/src/datatypes/mesh_properties.rs Outdated Show resolved Hide resolved
@teh-cmc
Copy link
Member Author

teh-cmc commented Oct 18, 2023

image

aren't these files marked as generated anymore? GitHub doesn't show the usual text as it does for other things:

That's weird, the gitattributes seem to be generated correctly... I'll have a closer look once I get a minute.

@teh-cmc teh-cmc mentioned this pull request Oct 18, 2023
Merged
4 tasks
emilk
emilk approved these changes Oct 18, 2023
View reviewed changes
rerun_cpp/src/rerun/blueprint/space_view_component.cpp
Comment on lines +23 to +25
arrow::MemoryPool *memory_pool
) {
if (!memory_pool) {
Copy link
Member

Choose a reason for hiding this comment

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

You are still on the old .clang-format file. I recently added DerivePointerAlignment: false to it that ensured pointer-stars get grouped with the type, so arrow::MemoryPool* memory_pool.

You should merge in main and run just codegen again before merging

Copy link
Member Author

Choose a reason for hiding this comment

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

But we are already on the up-to-date clang-format though, I have no idea what's going on...:
https://github.com/rerun-io/rerun/blob/cmc/arrow2convert_be_gone_3_for_real/.clang-format#L18

@teh-cmc teh-cmc merged commit 635ddeb into main Oct 18, 2023
31 of 35 checks passed
@teh-cmc teh-cmc deleted the cmc/arrow2convert_be_gone_3_for_real branch October 18, 2023 13:46
teh-cmc added a commit that referenced this pull request Oct 19, 2023
@teh-cmc
arrow2-convert migration 4: support serde-based types in codegen (#…
e537874 
…3902)

This introduces the `rust.attr.serde_type` attribute, allowing you to
use any `serde`-compatible Rust types in our IDL.
```
table ViewportLayout (
  "attr.rust.derive": "PartialEq",
  "attr.rust.override_crate": "re_viewport"
) {
  space_view_keys: [ubyte] (order: 100, "attr.rust.serde_type": "std::collections::BTreeSet<re_viewer_context::SpaceViewId>");

  tree: [ubyte] (order: 101, "attr.rust.serde_type": "egui_tiles::Tree<re_viewer_context::SpaceViewId>");

  auto_layout: bool (order: 102);
}
```

This unblocks further blueprint experimentations, and is the last
blocker to sunset `arrow2-convert`.

- `SpaceViewComponent`, `SpaceViewMaximized` & `ViewportLayout` are now
all implemented that way.
- `re_viewport` is now free of `arrow2-convert`.

---

`arrow2-convert` migration PR series:
- #3853 
- #3855
- #3897
- #3902
- #3917
teh-cmc added a commit that referenced this pull request Oct 19, 2023
@teh-cmc
arrow2-convert migration 5: sunset arrow2-convert (#3917)
26654cd 
The end of our wonderful journey.

- `NumInstances` control column now has an actual dedicated component
type.
- `EntityPath` is now a component.
- `Into<Cow<Self>>` impls have been cleaned up to generate way less
code.
- `arrow2_convert` is fully gone.


---

`arrow2-convert` migration PR series:
- #3853 
- #3855
- #3897
- #3902
- #3917
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilk emilk emilk approved these changes

Assignees
No one assigned
Labels
codegen/idl dependencies concerning crates, pip packages etc include in changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@teh-cmc @emilk