-
Notifications
You must be signed in to change notification settings - Fork 296
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
[VCPKG-ARTIFACTS] Fixes to make Espressif IDF work (with artifacts for dependencies) #604
[VCPKG-ARTIFACTS] Fixes to make Espressif IDF work (with artifacts for dependencies) #604
Conversation
Thanks to @fearthecowboy for help on this one. I considered ripping out the rush dependency, since it seems like we are only ever going to have one meaningful "project", and the dependency deduplication we need seems to be already done by pnpm (rather than npm), but after talking with @fearthecowboy I've decided to not go there since we still have rush linking the test project in. Unfortunately, there does not appear to be an effiicent way to build the typescript parts out-of-source, since they depend on node_modules which is put into the source tree. This adds a vcpkg.ps1 which does the same "environment hacking" as the in-development ce.ps1, teaches CMakeLists.txt to invoke rush and the typescript compiler as necessary, and teaches vcpkg.exe to use a hard-coded-into-the-binary path to the source tree when that in-development setting is turned on. The previous "always download latest ce bits" behavior is retained for folks who build vcpkg.exe from source and don't want to arrange for node and rush to be available.
…egistry zip files
ROFL it's actually "github.com/some-example" 🤣😂 |
# Conflicts: # CMakeLists.txt # src/vcpkg-in-development.ps1
Yeah, I got tired of not having a place for 1-off examples of stuff so I made a new org for myself. |
I merged 426593e then merged main into this to get the different-PR content out. |
@@ -232,6 +234,17 @@ export class Artifact extends ArtifactBase { | |||
this.session.activation.addExports(exportsBlock, this.targetLocation); | |||
} | |||
|
|||
// if espressif install | |||
if (this.metadata.info.flags.has('espidf')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that get/set property earlier in the review used now? Since this is reaching into flags directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! no, it's not -- the espidf flag there was when we had it nested in the git clone
..
I've deleted that code several times, but it keeps creeping back in when I pull from someone else 😢
lemme remove that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one doesn't look changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the correct usage, the other has been removed.
Co-authored-by: Billy O'Neal <[email protected]>
Co-authored-by: Billy O'Neal <[email protected]>
Co-authored-by: Billy O'Neal <[email protected]>
Co-authored-by: Billy O'Neal <[email protected]>
…cpkg-tool into espressif-update
@@ -232,6 +234,17 @@ export class Artifact extends ArtifactBase { | |||
this.session.activation.addExports(exportsBlock, this.targetLocation); | |||
} | |||
|
|||
// if espressif install | |||
if (this.metadata.info.flags.has('espidf')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one doesn't look changed?
Includes work from @BillyONeal 's fork for espressif.
To test this:
output