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

Snapshot creator in Rust #2617

Merged
merged 2 commits into from
Jul 8, 2019
Merged

Snapshot creator in Rust #2617

merged 2 commits into from
Jul 8, 2019

Conversation

ry
Copy link
Member

@ry ry commented Jul 6, 2019

This was referenced Jul 6, 2019
Closed
Closed
@ry ry force-pushed the feat-snapshot_creator_rust branch from aa2f271 to 0a2a24c Compare July 7, 2019 00:48
@bartlomieju
Copy link
Member

This looks very good to me, I have a couple of questions though:

  • I guess adding Rust as dependency for libdeno is no longer a concern because snapshot creator is now core/ concept?
  • how about renaming Snapshot1 and Snapshot2 from core/libdeno.rs while you're at it?

@ry
Copy link
Member Author

ry commented Jul 7, 2019

I guess adding Rust as dependency for libdeno is no longer a concern because snapshot creator is now core/ concept?

I changed the libdeno tests to construct its snapshot at runtime rather than compile time, to simplify the GN deps. This allows removing core/libdeno/snapshot_creator.cc and other code.

how about renaming Snapshot1 and Snapshot2 from core/libdeno.rs while you're at it?

This is another refactor that should be done separately. There's more to it than just renaming.

@ry ry force-pushed the feat-snapshot_creator_rust branch from 1dd94e9 to 49961d3 Compare July 7, 2019 18:17
@piscisaureus piscisaureus force-pushed the feat-snapshot_creator_rust branch from 556dfde to 919c5a1 Compare July 7, 2019 21:00
piscisaureus
piscisaureus reviewed Jul 7, 2019
View reviewed changes
@piscisaureus piscisaureus force-pushed the feat-snapshot_creator_rust branch 2 times, most recently from c39d905 to f60518e Compare July 7, 2019 22:27
@ry ry force-pushed the feat-snapshot_creator_rust branch 2 times, most recently from d2f08a5 to 1e28f41 Compare July 8, 2019 15:25
@piscisaureus @ry
Pass env vars through in build_extra/rust/run.py
94fe936 
This fixes an appveyor issue that arrises when implementing
snapshot_creator in Rust.
@ry ry force-pushed the feat-snapshot_creator_rust branch 2 times, most recently from 18f3915 to 4239f8b Compare July 8, 2019 16:06
piscisaureus
piscisaureus approved these changes Jul 8, 2019
View reviewed changes
Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

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

👍

@ry ry force-pushed the feat-snapshot_creator_rust branch from 9450635 to f74bc3f Compare July 8, 2019 21:55
@ry ry merged commit d641782 into denoland:master Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piscisaureus piscisaureus piscisaureus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ry @bartlomieju @piscisaureus