-
-
Notifications
You must be signed in to change notification settings - Fork 399
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
Update Theia to 1.22.1 #791
Conversation
@msujew I updated the CI updating node to 14, but there's an issue with I guess we can open a PR here and update to node 14
|
b78a73a
to
4e7306e
Compare
4e7306e
to
3118b4c
Compare
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.
Thanks for updating the ARM build documentation @fstasi!
I see two other places in the documentation where Node.js 12 is still specified:
docs/internal/Ubuntu.md
https://github.com/arduino/arduino-ide/blob/msujew/theia-update-1.22/docs/internal/Ubuntu.md
&& nvm install 12.14.1 \
&& nvm use 12.14.1 \
&& nvm alias default 12.14.1 \
BUILDING.md
Please refer to the Theia IDE prerequisites documentation for the setup instructions.
https://github.com/eclipse-theia/theia/blob/master/doc/Developing.md#prerequisites
- Node.js
>= 12.14.1
.
- For now, use Node 12.
I wonder if it would be best to specify the prerequisites directly in this repository's documentation?
The current approach of pointing to the Theia docs at the tip of the master
branch is a bad idea because that documentation is not guaranteed to be correct for the version of Theia used in this project. The alternative would be to use the ref for the version that is in use. In this case it would be:
https://github.com/eclipse-theia/theia/blob/v1.22.1/doc/Developing.md#prerequisites
and then remember to update the link at each bump. But that still assumes that those docs will always be correct for Arduino IDE.
448a676
to
4a4fbe6
Compare
@per1234 Thanks for pointing me out the Regarding the link to the Theia building instructions, I'm not sure if it would be better to integrate such extensive doc in our codebase and maintain it and where to place it so that it's available for ppl with issues building but not "too much" visible when developers just want to jump in a contribute. What is your take on that? |
@msujew we also noticed this error that breaks the menus (and potentially other stuff)
|
Unless I misunderstand, it is really not much content. This is the entirety of it:
Yeah, this would be the motivation for the current approach of referencing the Theia docs. We avoid the need to maintain a copy of that information here.
It depends on this question: "Is the Theia documentation accurate for the Arduino IDE project?" If the answer is "yes", then we should continue to reference it, but we should pin to the specific version that is accurate. If the answer is "no", then we should maintain our own build dependencies installation instructions. Since the Theia docs say Node.js 12, it seems to me that they are not accurate for Arduino IDE. |
@fstasi This is still work in progress, I'm currently trying to fix the build error in the MacOS CI. Given that you were able to run the electron build, did you not experience the build error? |
@msujew I confirm I was able to build and run the IDE on my intel mac laptop :). The build is succesful and the application starts, with the above run time error. As a side note, the build itself also works on the CI, my understanding is that the packaging of the electron executable is not working |
The question is really: where do we want to have this "technical" documentation about building/debugging and the FAQs |
Instructions for dependencies installation are essential to building, so I think the appropriate place for them is in the The information currently under the "Architecture overview" and "FAQ" is far less essential, and so would be reasonable to move to separate files that are linked to from |
The documentation updates I suggested are now done. Thanks!
@msujew thanks for the progress on this one! There is still some issue with the packaged version of the application: I get this error running the executable on mac:
|
@fstasi Yep, I noticed that one as well. I wanted to see whether simply disabling the Anyway, I talked with Paul about that, and he was quite confused as well, given that we don't rebuild |
@fstasi Would you mind testing the newest artifacts on MacOS? I believe it builds and runs correctly now. |
I still get the same error on the M1. What am I missing?
|
Nevermind, I forgot to add Nonetheless it still fails, seems like there are no native
|
@silvanocerza Well, that's a complicated one... So basically, it boils down to the The referenced issue explains how to build it locally, I'm not sure however, how well this integrates into the Theia build. I would try to get this PR merged regardless, since it's not a regression (I believe Theia didn't build on M1 before as well?) and not even really Theia related. |
That's a shame, does this also mean that developing the project on M1 is not feasible? |
Given what others have written in the linked issue, it should be possible to build the dependency yourself and instruct npm_config_grpc_tools_binary_host_mirror="https://github.com/maschwenk/grpc-node/raw/2dd28e1ab8211533007dd2df5ae632de60006983/artifacts/" yarn It shouldn't be a showstopper for you to release the Arduino-IDE on M1, but makes it a bit more difficult. |
Good to know, thanks Mark! |
I have been using this build for a few hours and can't say I have found any problem we don't already have in the main. |
# Conflicts: # arduino-ide-extension/src/electron-main/theia/electron-main-application.ts
afd5c6f
to
97d15f7
Compare
56a398b
to
505dd20
Compare
@msujew this PR looks good to me, anything pending before we can merge it? |
@fstasi This is ready to merge now :) I removed the |
The largest change in the update to Electron 15 is that
remote
is now exported from@electron/remote
instead ofelectron
directly.Please check whether the electron related functionality still works as expected. The application runs correctly on my machine with Electron 15.
Regarding the change in the readme: Theia 1.20 shipped with a fix for the
rebuild
issue, so it can be run from the root of the repo again.