-
Notifications
You must be signed in to change notification settings - Fork 33
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 environment? Cannot build on ubuntu. #590
Comments
Hey @davidfiala! Thank you for your interest. Our build environment currently is just NodeJS 18+ with Yarn. For now it runs on NodeJS versions 18 and 20. I haven't tried to run it on fresher versions yet. You may want to try to replicate the CI workflow locally.
I'd be glad to accept your contributions! |
Hi @aikoven Embarassed to admit it, but I've put in several hours looking at the setup and thinking about potential build improvements. It has led to several rabbit holes. I'd love to discuss alignment on some of them with you to see if you'd be open to accepting some wider ranging changes before I proceed at all. My biggest sticker is that it seems that the project does not compile in a vanilla environment, in my experience. That'd be fresh unadulterated node+npm installation without any global packages on the host system. I have not dived too deep in to why the CI build does work, but my benchmark here is that a recent fresh Ubuntu dev system will work, cross platform with Mac too. My other goal would be to get tsconfig aligned across the project and get vscode to play ball with the tsconfig. Right now, I think some of the src/file expressions preclude some files from properly being recognized as in the project or not, leading to red squiggles and inability to perform intellisense. Potential areas I'd love to align on, based on my rabbit holes so far:
As for switching to ESM everywhere, I think we can still output in any format if we had to. esbuild, etc could help if tsc gets annoying. It's kind of a loose set of notes/ideas.. sorry. But I've had to timebox myself, realizing that if I proceed you may not accept the changes since they'll become pretty large. It's getting a little hairy. I'm also not convinced that for the time I can give to help that I can chunk this in to many fine-grained commits. I'm absolutely swamped over at Teclada, but I do want to help nice-grpc in a better place.. especially with bugs like #555 looking very scary. I'm separately working on grpc-js bug fixes and I don't want to particularly risk being incompatible with nice-grpc. I'd be happy to jump on a chat anytime to discuss realtime, too. WDYT? |
Hey @davidfiala! I am not directly opposed to your modernization proposal, but are you sure all of this is required to fix build problems? What problem exactly you got with the build by the way? Below I will leave comments on each of the proposals. But something tells me that we can unlock your work on #555 with much much less blood.
What files are we talking about? I am using VSCode to develop nice-grpc and everything works just fine for me.
That would be nice! I would also consider migrating to npm, to simplify things. Using yarn is just my old-school habit.
The only package that uses google-protobuf in its implementation currently is the server reflection library. That's because reflection protobufs are written in protobuf v2, which at that time was supported by ts-proto only for parsing but not serialization. It would be great to migrate if that's possible now. Apart from that, we provide support for google-protobuf in nice-grpc and nice-grpc-web and I don't want to drop it, because it can aid the migration of old code to nice-grpc from grpc-js/grpc-web where google-protobuf is still often used.
It's fine for NodeJS, but most of our packages can be used in the browser and other platforms. That's why we use @tsconfig/recommended, which currently has es2016 target. I worry that this change could break someone.
I have some concerns about that. First of all, I don't see a way we could drop CommonJS output for now. This would be more or less acceptable if our only consumer was application code. But our packages are broadly used in libraries, and this change would force them to switch to ESM as well. Having dual output sounds better, but has a price of a more complicated build system. To be honest, I don't see benefits that would justify this change. To sum up, I am definitely up for modernizing things, especially if that leads to simplification. On the other hand, complicating things does not sound right to me, even if that leads to a more modern setup. That said, I sure hope that you acknowledge that all of the above is largely unrelated to #555.
|
Thanks for your reply. Definitely not trying to complicate anything. My aim is to get the basic repos building on stock Ubuntu and second to that ensure that the configs and dependencies for each package are setup so that vscode can also recognize everything as well. Re: things like #555, I agree its likely unrelated. I'm just unable to compile and contribute to anything at the moment. Just FYI, here's where I am starting and why I'm surprised that the CI is working.
I tried with node v20.14.0 and v18.20.3 just to be safe. corepack always pushes me to 4.2.2 by default citing Trying to resolve this led me down the rabbit hole of seeing many things to potentially improve along the way. Hence my original issue about making a BUILDING.md that works on common dev environments :) If you don't mind, could you post your mac's default yarn and node versions? Could you post if you have anything globally installed in your node installation? Thank you! |
Thanks for the reproduction steps! I see now that the problem is caused by yarn 4 producing a little different Here are the versions installed on my Mac: ❯ node --version
v18.20.2
❯ yarn --version
1.22.22 I checked the CI logs and it appears that GitHub I'm not sure whether it is possible to lock yarn to v1 via I also tried installing dependencies using |
Got it! Yes, this works! package.json added:
In my other projects using yarn 4.x, I often don't need to pass the path to the tools for things like proto compilers and plugins. Yarn seems to put it on the path for me. I think that's something we'll be able to take advantage of here if we upgrade yarn. Thank you for the help! |
Per discussion in deeplay-io#590 this version of yarn is known working
As we've merged the yarn version info in to master, I'll close this issue. In the future we can reference the discussion here as needed for updating any build environment or steps. Using Ubuntu 23.10 with nvm to manage node versions, I can build nice-grpc using node v18.x and v20.x and v22.x at this time. |
Hi,
I was hoping to take a look at #555 but I noticed that on a fresh Ubuntu 23.10 with node v20.13.1 installed freshly via nvm I was unable to get the project to build properly without lots of hacking. And even then, I cannot get tests working.
What is your current developmental workstation build setup? Does it work with a fresh version of node? (ie, does it work if you haven't done any global npm installs?)
Some immediate observations:
@types/node
seems needed, but isn't present in the main package.jsonpackageManager
is missing in package.json, but I do see ayarn.lock
fileI'd be happy to help contribute here (including starting with a
BUILDING.md
), but would appreciate some pointers to at least see what a working baseline build environment looks like. Thank you!The text was updated successfully, but these errors were encountered: