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.

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

[RELAY] Allow non-CSourceModules to compile #5884

Closed
wants to merge 1 commit into from

Conversation

mbaret
Copy link
Contributor

@mbaret mbaret commented Jun 22, 2020

A recent patch added a check so that external modules must be a CSourceModule. Generally that's not the case, so this patch instead queries whether or not the external module implements "get_const_vars".

A recent patch added a check so that external modules
must be a CSourceModule. Generally that's not the case,
so this patch instead queries whether or not the
external module implements "get_const_vars".

Change-Id: I2f022cb904d9636639132e26985f5c66876b48bb
@mbaret
Copy link
Contributor Author

mbaret commented Jun 22, 2020

cc @zhiics @lhutton1

Copy link
Member

@zhiics zhiics left a comment

Choose a reason for hiding this comment

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

I am aware of this. The reason why I have the check here is because we only have CSourceMoudule at the same. We are going to send a PR for json runtime to finish left TODOs in the previous PR with unit tests.

@mbaret
Copy link
Contributor Author

mbaret commented Jun 22, 2020

I think this is a cleaner alternative for now. Currently we can't rebase on top of your commit because of that CHECK. Our Ethos-N integration uses a completely custom runtime that doesn't need any constant metadata stored.

@zhiics
Copy link
Member

zhiics commented Jun 22, 2020

You can use our code here for a better fix:

zhiics@27ccdd2#diff-019ffa1b2b353a928839c9c809cb3992R61-R71

@mbaret
Copy link
Contributor Author

mbaret commented Jun 22, 2020

Seeing as that part doesn't look like it has dependencies on the JSON runtime stuff, could you publish it as a standalone PR and I'll abandon this one?

@mbaret
Copy link
Contributor Author

mbaret commented Jun 23, 2020

Superseded by #5885

@mbaret mbaret closed this Jun 23, 2020
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