-
Notifications
You must be signed in to change notification settings - Fork 15
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
Cargo-frc project generation and toolchain installation #92
base: master
Are you sure you want to change the base?
Cargo-frc project generation and toolchain installation #92
Conversation
…n for arm-unknown-linux-gnueabi
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.
Can be called directly as
cargo frc build --year <year>
. Will default to current year (2020) regardless of installation status.
I think it'd be better to have the year in the frc metadata instead. You'd need to update the dependencies anyway…
We should also set the other relevant environment variables so that things like the cc
crate will pick up the toolchain.
.open(path.join("src/main.rs")) | ||
.map_err(str_map("Could not open src/main.rs"))?; | ||
|
||
write!(main, "{}", DIGITAL_OUT_ROBOT).map_err(str_map("Could not write to src/main.rs"))?; |
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.
Shouldn't we write the timed robot template instead?
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.
I was considering having this selectable via a flag.
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.
Seems like a sensible thing to do. It should probably default to the timed robot template though.
//.ok_or("Can't determine home directory".to_owned()) | ||
} | ||
|
||
pub const TOOLCHAIN_URL_2019: &str = "https://github.com/wpilibsuite/toolchain-builder/releases/download/v2019-3/FRC-2019-Linux-Toolchain-6.3.0.tar.gz"; |
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 official install location for the toolchain changed for this year. I don't think it's worth trying to support the 2019 toolchain.
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, I missed that in the docs. I agree that it isn't necessary to support 2019, but it is useful to test multi-version support.
} | ||
|
||
const TOOLCHAIN_URL_2019: &str = "https://github.com/wpilibsuite/toolchain-builder/releases/download/v2019-3/FRC-2019-Mac-Toolchain-6.3.0.tar.gz"; | ||
const TOOLCHAIN_URL_2020: &str = "https://github.com/wpilibsuite/roborio-toolchain/releases/download/v2020-2/FRC-2020-Mac-Toolchain-7.3.0.tar.gz"; |
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.
This gets very repetitive… wouldn't it be sensible to build the URL from its components?
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 might be. I went with just directly including the URLs because the toolchain download location has changed twice in the past two years. Building from components would make it easier to install specific versions, or let the user override the version if a newer version is released midseason without needing to update cargo-frc itself. We could potentially even query the github api to see what releases are available, but I'm not sure if that is necessary. I'll think about this some more.
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.
I don't expect the toolchain repo to change location again.
cargo-frc/src/toolchain.rs
Outdated
if !Command::new("sh") | ||
.arg("-c") | ||
.arg(format!( | ||
"wget -c {} -O - | tar -xz -C {} --strip-components=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.
Yikes.
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.
I agree, but tar
needs to be called no matter what so it seemed worthwhile to use wget
to make downloading more convenient. The only thing I really don't like Is the amount of output from wget
, but I just found a way to display a progress bar without everything else, so I'll change to that.
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.
Surely there's a tar
crate?
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.
There is, but it does tar and nothing else, and requires some extra work to replicate --strip-components=1
. Downloading and installing without using sh
would require adding reqwest
(which could also bring async into the equation), flate2
, and tar
. Since cargo-frc
already heavily relies on calling unix commands instead of using libraries, I figured this would be a very convenient alternative. Do you think it's very important or advantageous to change to use these libraries?
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.
something something Windows
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.
Can be called directly as
cargo frc build --year <year>
. Will default to current year (2020) regardless of installation status.I think it'd be better to have the year in the frc metadata instead. You'd need to update the dependencies anyway…
We should also set the other relevant environment variables so that things like the
cc
crate will pick up the toolchain.
That's a good idea. I'll leave the year flag, but have it fall back to the metadata. Are there other variable besides CC
that you know of that should be set? I notice that both rustc
and cc
have ar
flags/vars, but I'm not sure if that is important.
Looking closer at the |
I don't see anything about |
Archives are always the same format regardless of architecture. |
@@ -181,7 +181,7 @@ fn ssh<T: AsRef<OsStr>>(target_address: &T, command: &str) -> Result<(), String> | |||
Ok(()) | |||
} | |||
|
|||
const DEPLOY_TARGET_TRIPLE: &str = "arm-unknown-linux-gnueabi"; | |||
const DEPLOY_TARGET_TRIPLE: &str = crate::build::ROBORIO_TARGET_TRIPLE; |
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.
Maybe we should move this up a module.
Hey @grebneerg, I'd like to get this in, and plan to use it in CI as well. Any chance you could get this updated? |
Yeah definitely. I got a bit distracted by mentoring my old team when build season started, and then got pretty busy with classes, but I'll try to spend some time this week finishing this up. |
@@ -91,11 +93,21 @@ pub fn get_config() -> Result<FrcConfig, String> { | |||
error!("Neither a team number or rio address was specified."); | |||
}; | |||
|
|||
debug!("year val: {:?}", frc.get("year")); |
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.
Did you mean to keep this 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.
Since it's 2021, might as well add support for grabbing the 2021 toolchain.
.get("year") | ||
.and_then(|v| v.as_u64()) | ||
.map(|i| i.to_string()) | ||
.and_then(|s| Toolchain::from_year(s.as_str())); |
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.
Instead of converting an integer to a string then effectively back, maybe from_year
should take an integer instead.
This PR addresses #87 and #82.
This has only been tested on Manjaro Linux.
cargo frc init <team number>
andcargo frc new <name> <team number>
for creating boilerplate sample projects.cargo frc toolchain list
to list available toolchains and their installation status (currently 2020 and 2019 are supported)cargo frc toolchain install <year>
to install the latest available toolchain release for the given year in the standard~/wpilib/$YEAR
..cargo/config
cargo frc build --year <year>
. Will default to current year (2020) regardless of installation status.--year
flag that defaults to the current year. This does prevent the use ofcargo frc deploy
with toolchains not installed bycargo frc
, which could potentially be solved by a--no-build
flag of some sort requiring a separate build step.Logical next steps could include making the wpilib install location configurable and adding custom toolchain support.