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

build_runners: Attempt to make GeneratedFile steps #1606

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

llogick
Copy link
Contributor

@llogick llogick commented Nov 16, 2023

  • Use original handle when trying to resolve field access, ie the microzig case, where a dep requests the app to import one of it's own app modules - app_dep.app_mod.some.

@Techatrix
Copy link
Member

  • Use original handle when trying to resolve field access, ie the microzig case, where a dep requests the app to import one of it's own app modules - app_dep.app_mod.some.

I am not familiar with how microzig works as a dependency. Could you give me a usage example of microzig that would showcase the issue that this PR addresses so I can figure out if this new root_handle is a good solution?

@llogick
Copy link
Contributor Author

llogick commented Nov 16, 2023

  • Use original handle when trying to resolve field access, ie the microzig case, where a dep requests the app to import one of it's own app modules - app_dep.app_mod.some.

I am not familiar with how microzig works as a dependency. Could you give me a usage example of microzig that would showcase the issue that this PR addresses so I can figure out if this new root_handle is a good solution?

Yes, certainly -

An example microzig app https://github.com/ZigEmbeddedGroup/microzig-examples/tree/main/espressif-esp
common pattern https://github.com/ZigEmbeddedGroup/microzig-examples/blob/04f2c4a6ff5f4b20f2848175082f98409ecc0ede/espressif-esp/src/blinky.zig#L2-L3
these are actually app's modules https://github.com/ZigEmbeddedGroup/microzig/blob/f8a506ccc28447ff6f20bba13ad04e7ce8d26318/src/microzig.zig#L13-L26

So the app imports the microzig dep which uses the app's modules in the context of the app.

The app would also work if those lines were part of the app, but it's probably a convenience thing.

@llogick llogick force-pushed the nullptrdevs/make-genfile-step+microzig branch from cbe3182 to d7db532 Compare November 21, 2023 19:08
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (d433a58) 73.79% compared to head (49a6b12) 73.77%.

Files Patch % Lines
src/analysis.zig 66.66% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1606      +/-   ##
==========================================
- Coverage   73.79%   73.77%   -0.02%     
==========================================
  Files          33       33              
  Lines        8974     8981       +7     
==========================================
+ Hits         6622     6626       +4     
- Misses       2352     2355       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/analysis.zig Outdated Show resolved Hide resolved
@llogick llogick force-pushed the nullptrdevs/make-genfile-step+microzig branch from d7db532 to 49a6b12 Compare November 23, 2023 00:26
@llogick llogick merged commit 38a1998 into master Nov 23, 2023
13 of 14 checks passed
@llogick llogick deleted the nullptrdevs/make-genfile-step+microzig branch November 23, 2023 00:30
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