-
Notifications
You must be signed in to change notification settings - Fork 36
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
Hide library that is only used by WASM builds behind a feature #157
Conversation
Should I bump the version? |
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.
It's used in WASM builds which fail if you remove it. If we're going to be excluding it to get around issues in other dependencies, we should move it to only WASM builds instead.
Adding it to
Caused my code to break again. Maybe we could put it behind a feature or something? Not sure what the best approach is here, I'm not super familiar with WASM builds. |
@rooooooooob I'm not really sure how to move forward on this. As I mentioned above, just adding it to the Wasm deps gave me an error:
A slightly different error from before, but still not happy. If you want to repro this, btw, you can
in the Do you have any suggestions? |
Even if I do this without the CML dependency (Just the Aiken one) I get:
despite it existing on crates.io as well as if I locally do I'm not sure what that is about, but from what you posted it sounds like moving it to WASM likely fixed the original issue, but now we have a different one with dependency conflicts. I can't even |
Very strange that you're having issues with |
(I can't repro the above case so I struck it out) Just for fun, I cut down the |
Okay. There is the option of pinning |
@rooooooooob |
Unrelated to my changes, there are a bunch of warnings from the rust compiler that are cluttering my compiles. I might clean those up as well in my PR, if that's okay. I could do it separately too. I'd like to get these changes through. |
package-lock.json
Outdated
@@ -1,1161 +1,8 @@ | |||
{ | |||
"name": "cardano-multiplatform-lib", | |||
"version": "3.1.1", | |||
"lockfileVersion": 2, | |||
"lockfileVersion": 1, |
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.
@SebastienGllmt Do you remember anything about this? I vaguely recall at some point it going from version 1 to 2 and adding a bunch of lines (like the ones removed here).
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.
Depending on which version of NPM you have installed, it will change the lockfileVersion. It's annoying because if you have multiple people on your team with different NPM versions, it causes issues. Maybe v2 has been out long enough that it's less of an issue now, but still annoying.
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.
> npm -v
8.5.4
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 this not going to work? Do I need to change something?
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.
you need to undo your changes to package-lock.json
and then run npm install again making sure you're on a recent enough version that supports the v2 format (8.5.4 should be recent enough)
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.
Kk. Ran that. I'm confused why it acted differently this time. I didn't change my npm
version. Maybe it was another script that messed it up before, like rust:build-nodejs
maybe.
@rooooooooob @SebastienGllmt any other changes? |
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.
LGTM, but @rooooooooob needs to sign off on this
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.
LGTM on my end too.
CML was causing some weird cyclical dependencies when used with other libraries that depended on
getrandom
:Removing thisgetrandom
from CML seems to fix it and CML still compiles.Update: This solution doesn't work because it breaks WASM builds. Instead I've moved the
getrandom
to the wasm deps and put it behind awasm
feature. This required me to add that feature to the WASM build scripts as well.