Skip to content

Commit

Permalink
configs: Remove alias resolver from cell instance
Browse files Browse the repository at this point in the history 
Summary: :celebrate:

Reviewed By: stepancheg

Differential Revision: D59725048

fbshipit-source-id: 9da442ae13fa481437aa7004b4498408ef915185
  • Loading branch information
@JakobDegen @facebook-github-bot
JakobDegen authored and facebook-github-bot committed Jul 15, 2024
1 parent ead0048 commit ddae6f9
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 70 deletions.
12 changes: 2 additions & 10 deletions app/buck2_common/src/legacy_configs/cells.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,8 +350,8 @@ impl BuckConfigBasedCells {

for (alias, destination) in Self::get_cell_aliases_from_config(&config)? {
let alias_path =
cells_aggregator.add_cell_alias(root_path.clone(), alias.clone(), destination)?;
root_aliases.insert(alias, alias_path.clone());
cells_aggregator.add_cell_alias_for_root_cell(alias.clone(), destination)?;
root_aliases.insert(alias, alias_path);
}

if let Some(external_cells) = config.get_section("external_cells") {
Expand Down Expand Up @@ -390,14 +390,6 @@ impl BuckConfigBasedCells {
)
.await?;

for (alias, alias_path) in &root_aliases {
cells_aggregator.add_cell_entry(path.clone(), alias.clone(), alias_path.clone())?;
}

for (alias, destination) in Self::get_cell_aliases_from_config(&config)? {
cells_aggregator.add_cell_alias(path.clone(), alias.clone(), destination)?;
}

buckconfigs.insert(path, config);
}

Expand Down
81 changes: 37 additions & 44 deletions app/buck2_core/src/cells.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,7 @@ impl CellResolver {
#[derive(Debug)]
pub struct CellsAggregator {
cell_infos: HashMap<CellRootPathBuf, CellAggregatorInfo>,
root_aliases: HashMap<NonEmptyCellAlias, CellRootPathBuf>,
}

#[derive(Debug, Default)]
Expand All @@ -495,31 +496,14 @@ struct CellAggregatorInfo {
/// So that it is predictable, we always use the first name we encounter,
/// so the root file can choose what the alias is called.
name: Option<CellName>,
/// All the aliases known by this cell.
alias_mapping: HashMap<NonEmptyCellAlias, CellRootPathBuf>,
external: Option<ExternalCellOrigin>,
}

impl CellAggregatorInfo {
fn add_alias_mapping(
&mut self,
from: NonEmptyCellAlias,
to: CellRootPathBuf,
) -> anyhow::Result<()> {
let old = self.alias_mapping.insert(from.clone(), to.clone());
if let Some(old) = old {
if old != to {
return Err(CellError::DuplicateAliases(from, old, to).into());
}
}
Ok(())
}
}

impl CellsAggregator {
pub fn new() -> Self {
Self {
cell_infos: HashMap::new(),
root_aliases: HashMap::new(),
}
}

Expand Down Expand Up @@ -547,26 +531,41 @@ impl CellsAggregator {
if name.is_none() {
*name = Some(CellName::unchecked_new(parsed_alias.as_str())?);
}
self.cell_info(cell_root)
.add_alias_mapping(parsed_alias, alias_path)

if cell_root.is_repo_root() {
self.add_cell_alias_for_root_cell_inner(parsed_alias, alias_path)?;
}
Ok(())
}

/// Adds a cell alias configuration entry
pub fn add_cell_alias(
pub fn add_cell_alias_for_root_cell(
&mut self,
cell_root: CellRootPathBuf,
parsed_alias: NonEmptyCellAlias,
alias_destination: NonEmptyCellAlias,
) -> anyhow::Result<CellRootPathBuf> {
let cell_info = self.cell_info(cell_root);
let alias_path = match cell_info.alias_mapping.get(&alias_destination) {
let alias_path = match self.root_aliases.get(&alias_destination) {
None => return Err(CellError::AliasOnlyCell(parsed_alias, alias_destination).into()),
Some(alias_path) => alias_path.clone(),
};
cell_info.add_alias_mapping(parsed_alias, alias_path.clone())?;
self.add_cell_alias_for_root_cell_inner(parsed_alias, alias_path.clone())?;
Ok(alias_path)
}

fn add_cell_alias_for_root_cell_inner(
&mut self,
from: NonEmptyCellAlias,
to: CellRootPathBuf,
) -> anyhow::Result<()> {
let old: Option<CellRootPathBuf> = self.root_aliases.insert(from.clone(), to.clone());
if let Some(old) = old {
if old != to {
return Err(CellError::DuplicateAliases(from, old, to).into());
}
}
Ok(())
}

fn get_cell_name_from_path(&self, path: &CellRootPath) -> anyhow::Result<CellName> {
self.cell_infos
.get(path)
Expand All @@ -583,7 +582,7 @@ impl CellsAggregator {
}

/// Creates the 'CellResolver' from all the entries that were aggregated
pub fn make_cell_resolver(self) -> anyhow::Result<CellResolver> {
pub fn make_cell_resolver(mut self) -> anyhow::Result<CellResolver> {
let mut cell_mappings = Vec::new();

let all_cell_roots_for_nested_cells: Vec<_> = self
Expand All @@ -592,39 +591,35 @@ impl CellsAggregator {
.map(|path| Ok((self.get_cell_name_from_path(path)?, path.as_path())))
.collect::<anyhow::Result<_>>()?;

let mut root_cell_alias_resolver = None;
let mut root_cell_name = None;

for (cell_path, cell_info) in &self.cell_infos {
let nested_cells =
NestedCells::from_cell_roots(&all_cell_roots_for_nested_cells, cell_path);

let mut aliases_for_cell = HashMap::new();
let cell_name = self.get_cell_name_from_path(cell_path)?;

for (alias, path_for_alias) in &cell_info.alias_mapping {
aliases_for_cell
.insert(alias.clone(), self.get_cell_name_from_path(path_for_alias)?);
}

let alias_resolver = CellAliasResolver::new(cell_name, aliases_for_cell)?;

if cell_path.is_repo_root() {
root_cell_alias_resolver = Some(alias_resolver.clone());
root_cell_name = Some(cell_name);
}

cell_mappings.push(CellInstance::new(
cell_name,
cell_path.clone(),
cell_info.external.dupe(),
alias_resolver,
nested_cells,
)?);
}

CellResolver::new(
cell_mappings,
root_cell_alias_resolver.ok_or(CellError::NoRootCell)?,
)
let root_cell_name = root_cell_name.ok_or(CellError::NoRootCell)?;
let mut root_aliases = HashMap::new();
for (alias, path) in std::mem::take(&mut self.root_aliases) {
root_aliases.insert(alias, self.get_cell_name_from_path(&path)?);
}

let root_cell_alias_resolver = CellAliasResolver::new(root_cell_name, root_aliases)?;

CellResolver::new(cell_mappings, root_cell_alias_resolver)
}

pub fn mark_external_cell(
Expand Down Expand Up @@ -779,10 +774,8 @@ mod tests {
fn test_alias_only_error() -> anyhow::Result<()> {
let mut agg = CellsAggregator::new();

let cell_root = CellRootPathBuf::new(ProjectRelativePathBuf::try_from("".to_owned())?);
assert!(
agg.add_cell_alias(
cell_root,
agg.add_cell_alias_for_root_cell(
NonEmptyCellAlias::new("root".to_owned()).unwrap(),
NonEmptyCellAlias::new("does_not_exist".to_owned()).unwrap(),
)
Expand Down
16 changes: 0 additions & 16 deletions app/buck2_core/src/cells/instance.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,9 @@ use crate::cells::cell_root_path::CellRootPathBuf;
use crate::cells::external::ExternalCellOrigin;
use crate::cells::name::CellName;
use crate::cells::nested::NestedCells;
use crate::cells::CellAliasResolver;

#[derive(Debug, buck2_error::Error)]
enum CellInstanceError {
#[error("Inconsistent cell name: `{0}` in instance, but `{1}` in alias resolver")]
InconsistentCellName(CellName, CellName),
#[error(
"Attempted to refer to cell `{0}`; however, this is an external cell which cannot be used from `{1}`"
)]
Expand All @@ -45,9 +42,6 @@ struct CellData {
/// the project relative path to this 'CellInstance'
path: CellRootPathBuf,
external: Option<ExternalCellOrigin>,
#[derivative(Debug = "ignore")]
/// the aliases of this specific cell
aliases: CellAliasResolver,
nested_cells: NestedCells,
}

Expand All @@ -56,12 +50,8 @@ impl CellInstance {
name: CellName,
path: CellRootPathBuf,
external: Option<ExternalCellOrigin>,
aliases: CellAliasResolver,
nested_cells: NestedCells,
) -> anyhow::Result<CellInstance> {
if name != aliases.current {
return Err(CellInstanceError::InconsistentCellName(name, aliases.current).into());
}
if external.is_some()
&& let Some(nested) = nested_cells.check_empty()
{
Expand All @@ -71,7 +61,6 @@ impl CellInstance {
name,
path,
external,
aliases,
nested_cells,
})))
}
Expand All @@ -88,11 +77,6 @@ impl CellInstance {
&self.0.path
}

#[inline]
pub fn testing_cell_alias_resolver(&self) -> &CellAliasResolver {
&self.0.aliases
}

#[inline]
pub fn nested_cells(&self) -> &NestedCells {
&self.0.nested_cells
Expand Down

0 comments on commit ddae6f9

Please sign in to comment.