Skip to content

Commit

Permalink
Download both the release artifacts in extended mode for LLVM >= 17
Browse files Browse the repository at this point in the history
The Espressif-provided LLVM toolchain no longer has the libs-only
tarball as a strict subset of the full tarball.  Instead the "full"
tarball now contains a clang with libclang built in.  To work around
this we now need to get _both_ tarballs if we're in extended LLVM mode.

Cf. espressif/llvm-project#99

Signed-off-by: Johannes Löthberg <johannes.loethberg@elokon.com>
  • Loading branch information
kyrias authored and SergioGasquez committed Jun 28, 2024
1 parent 84081c2 commit 9c0234d
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 18 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Added

### Fixed
- Make both `libclang.so` available again when installing the extended LLVM for LLVM versions >= 17 (#432)

### Changed

Expand Down
59 changes: 41 additions & 18 deletions src/toolchain/llvm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@ pub const CLANG_NAME: &str = "xtensa-esp32-elf-clang";
pub struct Llvm {
// /// If `true`, full LLVM, instead of only libraries, are installed.
extended: bool,
/// LLVM Toolchain file name.
pub file_name: String,
/// LLVM libs-only toolchain file name.
pub file_name_libs: String,
/// LLVM "full" toolchain file name.
pub file_name_full: Option<String>,
/// Host triple.
pub host_triple: HostTriple,
/// LLVM Toolchain path.
Expand Down Expand Up @@ -124,29 +126,40 @@ impl Llvm {
"llvm-"
};

let mut file_name = format!(
"{}{}-{}.tar.xz",
name,
version,
Self::get_arch(host_triple, &version)
);
if !extended {
if version != DEFAULT_LLVM_17_VERSION {
file_name = format!("libs_{file_name}");
let (file_name_libs, file_name_full) = {
let file_name_full = format!(
"{}{}-{}.tar.xz",
name,
version,
Self::get_arch(host_triple, &version)
);

let file_name_libs = if version != DEFAULT_LLVM_17_VERSION {
format!("libs_{file_name_full}")
} else {
file_name = format!("libs-{}", file_name);
}
}
format!("libs-{file_name_full}")
};

let repository_url = format!("{DEFAULT_LLVM_REPOSITORY}/{version}/{file_name}");
(
file_name_libs,
if version == DEFAULT_LLVM_15_VERSION || version == DEFAULT_LLVM_16_VERSION {
None
} else {
extended.then_some(file_name_full)
},
)
};

let repository_url = format!("{DEFAULT_LLVM_REPOSITORY}/{version}");
#[cfg(unix)]
let path = toolchain_path.join(CLANG_NAME).join(&version);
#[cfg(windows)]
let path = toolchain_path.join(CLANG_NAME);

Ok(Self {
extended,
file_name,
file_name_libs,
file_name_full,
host_triple: host_triple.clone(),
path,
repository_url,
Expand Down Expand Up @@ -248,13 +261,23 @@ impl Installable for Llvm {
} else {
info!("Installing Xtensa LLVM");
download_file(
self.repository_url.clone(),
"idf_tool_xtensa_elf_clang.tar.xz",
format!("{}/{}", self.repository_url, self.file_name_libs),
"idf_tool_xtensa_elf_clang.libs.tar.xz",
self.path.to_str().unwrap(),
true,
false,
)
.await?;
if let Some(file_name_full) = &self.file_name_full {
download_file(
format!("{}/{}", self.repository_url, file_name_full),
"idf_tool_xtensa_elf_clang.full.tar.xz",
self.path.to_str().unwrap(),
true,
false,
)
.await?;
}
}
// Set environment variables.
#[cfg(windows)]
Expand Down

7 comments on commit 9c0234d

@svenstaro
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SergioGasquez Any chance for a release with this inside?

@SergioGasquez
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I have plans on releasing it next week, is there any need for it earlier?

@svenstaro
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's currently broken and it's breaking previous behavior. Personally, I'd appreciate a hotfix release.

@SergioGasquez
Copy link
Member

@SergioGasquez SergioGasquez commented on 9c0234d Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, but is there any reason not to use cargo install espup --git https://github.com/esp-rs/espup in the meantime? If there's a specific reason to avoid this, I can proceed with a release now. We're aiming to hold off until next week to align with the Xtensa Rust release, and to avoid potentially needing two releases if any additional features or bug fixes arise.

@svenstaro
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm talking from the perspective of someone maintaining this in Arch. I guess I could just backport it, though.

@SergioGasquez
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! That completely makes sense! espup@0.12.1 was just released! Sorry for the delay, I had a few days of vacation last week.

@svenstaro
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok nice, thanks.

Please sign in to comment.