Add Redox OS compilation to Travis using Redoxer#1208
Conversation
5f8be00 to
04a8839
Compare
7245643 to
f7246a3
Compare
| # Uncommment when you want to compile/check with redoxer | ||
| # [patch.crates-io] | ||
| [patch.crates-io] | ||
| # REDOX START, Uncommment when you want to compile/check with redoxer |
There was a problem hiding this comment.
Maybe a patch file on Cargo.toml in the redox folder might be a better option here?
There was a problem hiding this comment.
If it were a patch file, it would only work with the same number of lines; it would break if we were to add more lines e.g. dependencies above where it's switching the lines.
There was a problem hiding this comment.
I would prefer doing a target dependent dependency like in the vm Cargo.toml. Some thing like
[target.'cfg(not(target_os = "redox"))'.dependencies] and [target.'cfg(target_os = "redox")'.dependencies]
There was a problem hiding this comment.
I tried doing target.cfg().patch syntax at first, but it didn't work for some reason.
There was a problem hiding this comment.
Why do we need to use the target.cfg().patch and not target.cfg().dependencies?
There was a problem hiding this comment.
This might be a nice option, so we can use feature flags, or even let cargo figure out the os, and use the target_os flag.
|
I'm not too sure about this change. We should try to keep the pull request builds light weight. Is there an option to run this check once in a while? We could use cron or something to run this now and then? Also, is there a redox capable CI-service? |
|
The funny thing is that the redox job is actually shorter than both the rust test and Python test jobs; also, I was thinking of opening a merge request to redox to add RustPython to their cookbook, and if we do do that it'd be good to make sure that master always compiles for redox. Although maybe we could base it off release, in which case I could just run this test if we're on release. |
| # Uncommment when you want to compile/check with redoxer | ||
| # [patch.crates-io] | ||
| [patch.crates-io] | ||
| # REDOX START, Uncommment when you want to compile/check with redoxer |
There was a problem hiding this comment.
I would prefer doing a target dependent dependency like in the vm Cargo.toml. Some thing like
[target.'cfg(not(target_os = "redox"))'.dependencies] and [target.'cfg(target_os = "redox")'.dependencies]
I agree with @windelbouwman. I don't think at this stage we should support another target. In case we decide to add another target to the CI I would suggest |
|
I did set it to only run as a cron job or when on the release branch, does that seem better? |
I am not sure this will be any different from the current status. Changes that break |
|
It would at least signal that something is broken. Also, as I mentioned before, it really does finish quickly, so if we wanted to do it every run we could; I could also change Also, re: failing on release, we could commit a fix to master through a quick PR when the merge master into release build fails, and then merge to release. |
|
I would suggest doing only the cron job and remove the release check. I think a release should depend only on supported targets. |
|
Maybe I could manage a redox-release branch or something that could be the source for the redox recipe. |
| fn extend_module_platform_specific(vm: &VirtualMachine, module: PyObjectRef) -> PyObjectRef { | ||
| let ctx = &vm.ctx; | ||
|
|
||
| #[cfg(not(target_os = "redox"))] |
There was a problem hiding this comment.
You might consider modifying line 443 somewhat more above. This function will then be enabled for not unix or redox.
There was a problem hiding this comment.
No, that's intentional. I'm assuming in the future that there will be unix-only parts of the module that are compatible with redox, so once those are added they can all stay in the unix extend_module_platform_specific fn like for os.
Redoxer