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

Move cruntime to separate module #31

Merged
merged 6 commits into from
Dec 5, 2018
Merged

Move cruntime to separate module #31

merged 6 commits into from
Dec 5, 2018

Conversation

noahemmet
Copy link
Contributor

This moves cruntime to a separate module, which allows this framework to be built on Linux. Discussion: #18 (comment) and noahemmet@8d685eb

  • It looked like the last few commits included *.xcodeproj and *.xcworkspace files, but if you'd like, I'm happy to add them to the .gitignore.
  • Tests pass locally on my Mac but I'm not set it up test on Linux; is that something Travis will do automatically?

Thanks for everyone's help on this!

noahemmet
noahemmet commented Nov 20, 2018
View reviewed changes
Runtime/Utilities/GetFieldAt.swift
let infoContext = ctx.assumingMemoryBound(to: PropertyInfoContext.self).mutable
let pointer = unsafeBitCast(type, to: UnsafeRawPointer.self)
swift_getFieldAt(pointer, UInt32(index), { name, type, ctx in
guard let name = name, let ctx = ctx else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this look correct? Should this be a fatalError instead perhaps? I'm not sure under what circumstances these would be nil.

Copy link
Owner

Choose a reason for hiding this comment

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

Yea I would do a fatalError instead. It shouldn't ever be nil and if it is it will fail somewhere else because of it.

noahemmet referenced this pull request in noahemmet/Runtime Nov 20, 2018
@noahemmet
Separated cruntime module
8d685eb
@noahemmet
Copy link
Contributor Author

I was getting a error: /Users/travis/build/wickwirew/Runtime/cruntime has no manifest in Travis so I added a Package.swift in the cruntime directory. The MacOS tests now pass but Linux is giving me a error: cannot find -lcruntime.

@wickwirew
Copy link
Owner

Not entirely sure. I don't have much experience with SwiftPM. My guess is the Runtime package needs the CRuntime package as a dependency

@wickwirew
Copy link
Owner

It doesn't look like you can have 2 packages in the same repo as explained here. I can create a new repo for the package if need be

@noahemmet
Copy link
Contributor Author

Ah would you mind? I can PR cruntime against it.

PS, would you prefer this be named cruntime or CRuntime? I wasn't sure about prior art here.

@wickwirew
Copy link
Owner

Would prefer CRuntime just to keep it consistent if you wouldn't mind updating it. And created it https://github.com/wickwirew/CRuntime

@noahemmet noahemmet mentioned this pull request Nov 30, 2018
Merged
@noahemmet
Copy link
Contributor Author

Hey, tests pass! I've updated the dependency to point to wickwirew/CRuntime.git, so feel free to re-run them after wickwirew/CRuntime#1 is merged.

A few things:

Thanks again to everyone who helped!

@noahemmet
Copy link
Contributor Author

I've verified this works by pushing a GraphQL app to Vapor's Cloud service here: https://graphql-star-wars.v2.vapor.cloud/graphiql

@wickwirew
Copy link
Owner

Sorry for the late response! Slipped my mind. Thanks again for tackling this!

@wickwirew wickwirew merged commit 6ad626f into wickwirew:master Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wickwirew wickwirew wickwirew left review comments

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@noahemmet @wickwirew