-
Notifications
You must be signed in to change notification settings - Fork 346
[CI] [Rust] Add-back the Rust image to CI #343
Conversation
5254d90
to
0da4bbb
Compare
0da4bbb
to
0920233
Compare
My only suggestion would be to change the rust channel to "nightly". Pinning to a specific date will fail as soon as the version becomes unavailable. |
Thanks for the feedback. Do you know if we can expect that there could eventually be breaking changes if we use "nightly". e.g. it going to the next major version, breaking some backward compatibility and necessitating that we update the image? |
I'm not developing on rust so I don't have an answer. Anyway, the stable channel is not building. We can only choose between pinned or unpinned nightly. I would go for the unpinned nightly and evaluate over time whether it's stable enough. My main argumentation is:
Any rust developer amongst us that would like to share his/her thoughts? |
cc: @dwjbosman Maybe you have some feedback about the rust channel discussion? |
Thanks @emdfonseca - I am also not a rust developer but your arguments make sense to me. I have pinged the original author of the rust image - I hope he might have some valuable feedback as well.
This is generally true but I think with one exception: the nightly build. It permits having a version of the images, built against the most recent eclipse-theia "master" branch. I do not think this invalidates your arguments however.
I am willing to give this a try 👍 |
Also: - Removed deprecated @theia/editorconfig - Added environment variable so that plugins (VS Code extensions) defined in package.json are detected/used Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
0920233
to
c6704e7
Compare
CI has passed with the latest PR version. @emdfonseca so far so good about using "nightly" channel - let's keep it like that and see how long it works. I have also addressed your other comments. @vince-fugnitto you want to have a look? There are not much 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.
Looks good! Thanks!
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.
The changes look good, I can see that the build is successfully passing as well 👍
Thanks for the help @emdfonseca. I noticed after the fact that you got started by asking a question on our Spectrum forum, and then you came here to submit a PR related to your question, rather then waiting for someone else to take care of the task. I think that's awesome. I wanted to ask if you have a general interest in Theia applications or if this was a little detour you needed to make in order to be able to fulfil a use-case of yours? If you have a general interest, I can add you as contributor to the repo and you'd be welcome to help if you like. Please let us know. |
Hi @marcdumais-work, it was mostly a consequence of my continuous journey of learning and experimenting. :) I might come back from time to time to support the project but nothing planned so far. If I end up supporting more I will come back to you. Thanks! |
Understood, thanks @emdfonseca |
We've been having issues for a while with the Rust image in CI. We're temporarily removed it so that CI can pass, but would like to being it back when the issues are fixed.
For a while, it looked like the Rust tools were causing the CI issue. A recent update seems to have remove this problem: #330 (comment) . So I thought I would give it a try and see what other issues there might be.
In addition to adding the image back into CI, this PR: