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

Include Static Resources in Example Project #105

Merged
merged 1 commit into from
Mar 21, 2020

Conversation

vbrandl
Copy link
Contributor

@vbrandl vbrandl commented Mar 20, 2020

This shows an example usage of localPatterns to include static resources in the build sandbox

@ebkalderon
Copy link
Member

Thanks for opening this PR, @vbrandl! While I do appreciate the addition, and the changes themselves do look correct to me, I don't think we should add this to either of the existing example projects. I think it's out of scope given both of their defined purposes and perhaps would be better suited to a later example with more practical use, e.g. an example use of tonic which includes external .proto schemas. Then again, that's just my thought. 😄

What do all you think, @trha @eupn @antigravityla @snawaz?

@vbrandl
Copy link
Contributor Author

vbrandl commented Mar 20, 2020

I could also create a third example repo and use https://github.com/vbrandl/cargo2nix-ructe-testcase as a base.

Also is there a better way to extend localPatterns? Currently I duplicated the default values. Maybe there is a way to append to the existing localPatterns instead of copying the default values?

@ebkalderon
Copy link
Member

ebkalderon commented Mar 20, 2020

That could work, sure! We'd need to write up a suitable tutorial for it, of course, but the Rust and Nix code for it is simple enough that I could see it being a good example project. 👍

Sadly, there isn't a way to duplicate the values for localPatterns under the current design, unless we were to maybe have [ ''^(src)(/.*)?'' ''[^/]*\.(rs|toml)$'' ] be built into makePackageSet and then have localPatterns ? [], which is then concatenated to that built-in list.

@vbrandl
Copy link
Contributor Author

vbrandl commented Mar 20, 2020

I reverted the changes in the existing example project and added a third example.

I don't know enough about nix expressions yet to implement the change you proposed for makePackageSet. Do you want to make this change on the same branch or will this be a separate PR?

@ebkalderon
Copy link
Member

I think it should be on a separate branch, since it's conceptually a different change that will need to be discussed and evaluated separately.

I'll review it as soon as I can, but I'm currently away from a computer at the moment. Please bear with me for a little bit! If someone else wants to swoop in and take this on, that'd be cool too.

ebkalderon
ebkalderon previously approved these changes Mar 21, 2020
Copy link
Member

@ebkalderon ebkalderon left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, @vbrandl. Looks good to me! 👍

Would you mind rebasing against the latest master and dropping the first few commits, including the reverts, from the PR?

@vbrandl
Copy link
Contributor Author

vbrandl commented Mar 21, 2020

I rebased against upstream master and removed the old commits.

Copy link
Member

@ebkalderon ebkalderon left a comment

Choose a reason for hiding this comment

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

Thanks for the great work, @vbrandl! Looks good to me.

I'll open an issue for writing up a proper tutorial for it.

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.

None yet

2 participants