Skip to content
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

fix: Fix self-referencing tasks using project aliases. #1082

Merged
merged 2 commits into from
Sep 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions crates/cli/src/commands/task.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,15 @@ pub struct TaskArgs {

#[system]
pub async fn task(args: ArgsRef<TaskArgs>, workspace: ResourceMut<Workspace>) {
let Some(project_id) = args.target.scope_id.clone() else {
let Some(project_locator) = args.target.scope_id.clone() else {
return Err(miette!("A project ID is required."));
};

let mut project_graph_builder = build_project_graph(workspace).await?;
project_graph_builder.load(&project_id).await?;
project_graph_builder.load(&project_locator).await?;

let project_graph = project_graph_builder.build().await?;
let project = project_graph.get(&project_id)?;
let project = project_graph.get(&project_locator)?;
let task = project.get_task(&args.target.task_id)?;

if args.json {
Expand All @@ -41,7 +41,7 @@ pub async fn task(args: ArgsRef<TaskArgs>, workspace: ResourceMut<Workspace>) {
term.line("")?;
term.render_label(Label::Brand, &args.target.id)?;
term.render_entry("Task", color::id(&args.target.task_id))?;
term.render_entry("Project", color::id(&project_id))?;
term.render_entry("Project", color::id(&project.id))?;
term.render_entry("Platform", term.format(&task.platform))?;
term.render_entry("Type", term.format(&task.type_of))?;

Expand Down
8 changes: 4 additions & 4 deletions crates/core/dep-graph/src/dep_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,8 @@ impl<'ws> DepGraphBuilder<'ws> {
color::label(target),
);

if let TargetScope::Project(project_id) = &target.scope {
let project = self.project_graph.get(project_id)?;
if let TargetScope::Project(project_locator) = &target.scope {
let project = self.project_graph.get(project_locator)?;
let dependents = self.project_graph.dependents_of(&project)?;

for dependent_id in dependents {
Expand Down Expand Up @@ -246,8 +246,8 @@ impl<'ws> DepGraphBuilder<'ws> {
return Err(TargetError::NoDepsInRunContext.into());
}
// project:task
TargetScope::Project(project_id) => {
let project = self.project_graph.get(project_id)?;
TargetScope::Project(project_locator) => {
let project = self.project_graph.get(project_locator)?;
let task = project.get_task(&target.task_id)?;

if let Some(index) =
Expand Down
4 changes: 2 additions & 2 deletions crates/core/runner/src/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -409,9 +409,9 @@ impl<'a> Runner<'a> {
return Ok(true);
}
}
TargetScope::Project(project_id) => {
TargetScope::Project(project_locator) => {
if let Some(owner_id) = &task.target.scope_id {
if owner_id == project_id && is_matching_task {
if owner_id == project_locator && is_matching_task {
return Ok(true);
}
}
Expand Down
1 change: 0 additions & 1 deletion crates/core/terminal/src/theme.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,5 @@ pub fn create_theme() -> ColorfulTheme {
.for_stderr()
.color256(Color::Teal as u8),
unpicked_item_prefix: style(" ".to_string()).for_stderr(),
..ColorfulTheme::default()
}
}
8 changes: 8 additions & 0 deletions nextgen/project-builder/src/project_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ pub struct ProjectBuilder<'app> {
// Values to be continually built
id: &'app Id,
source: &'app WorkspaceRelativePath,
alias: Option<&'app str>,
project_root: PathBuf,

pub language: LanguageType,
Expand All @@ -64,6 +65,7 @@ impl<'app> ProjectBuilder<'app> {
context,
id,
source,
alias: None,
global_config: None,
local_config: None,
language: LanguageType::Unknown,
Expand Down Expand Up @@ -188,9 +190,15 @@ impl<'app> ProjectBuilder<'app> {
self
}

pub fn set_alias(&mut self, alias: &'app str) -> &mut Self {
self.alias = Some(alias);
self
}

#[tracing::instrument(name = "project", skip_all)]
pub async fn build(mut self) -> miette::Result<Project> {
let mut project = Project {
alias: self.alias.map(|a| a.to_owned()),
dependencies: self.build_dependencies()?,
file_groups: self.build_file_groups()?,
tasks: self.build_tasks().await?,
Expand Down
7 changes: 3 additions & 4 deletions nextgen/project-expander/src/tasks_expander.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,16 +156,15 @@ impl<'graph, 'query> TasksExpander<'graph, 'query> {
}
}
// id:task
TargetScope::Project(project_id) => {
if project_id == &project.id {
TargetScope::Project(project_locator) => {
if project.matches_locator(project_locator) {
if dep_target.task_id == task.id {
// Avoid circular references
} else {
check_and_push_dep(project, &dep_target.task_id, false)?;
}
} else {
let results =
(self.context.query)(format!("project={id}", id = project_id))?;
let results = (self.context.query)(format!("project={}", project_locator))?;

if results.is_empty() {
return Err(TasksExpanderError::UnknownTarget {
Expand Down
8 changes: 4 additions & 4 deletions nextgen/project-graph/src/project_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,15 +106,15 @@ impl ProjectGraph {

/// Return a project with the provided name or alias from the graph.
/// If the project does not exist or has been misconfigured, return an error.
pub fn get(&self, alias_or_id: &str) -> miette::Result<Arc<Project>> {
pub fn get(&self, project_locator: &str) -> miette::Result<Arc<Project>> {
let mut boundaries = ExpansionBoundaries::default();

self.internal_get(alias_or_id, &mut boundaries)
self.internal_get(project_locator, &mut boundaries)
}

/// Return an unexpanded project with the provided name or alias from the graph.
pub fn get_unexpanded(&self, alias_or_id: &str) -> miette::Result<&Project> {
let id = self.resolve_id(alias_or_id);
pub fn get_unexpanded(&self, project_locator: &str) -> miette::Result<&Project> {
let id = self.resolve_id(project_locator);

let node = self
.nodes
Expand Down
30 changes: 17 additions & 13 deletions nextgen/project-graph/src/project_graph_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,8 @@ impl<'app> ProjectGraphBuilder<'app> {
}

/// Load a single project by name or alias into the graph.
pub async fn load(&mut self, alias_or_id: &str) -> miette::Result<()> {
self.internal_load(alias_or_id, &mut FxHashSet::default())
pub async fn load(&mut self, project_locator: &str) -> miette::Result<()> {
self.internal_load(project_locator, &mut FxHashSet::default())
.await?;

Ok(())
Expand All @@ -208,10 +208,10 @@ impl<'app> ProjectGraphBuilder<'app> {
#[async_recursion]
async fn internal_load(
&mut self,
alias_or_id: &str,
project_locator: &str,
cycle: &mut FxHashSet<Id>,
) -> miette::Result<NodeIndex> {
let id = self.resolve_id(alias_or_id);
let id = self.resolve_id(project_locator);

// Already loaded, exit early with existing index
if let Some(index) = self.nodes.get(&id) {
Expand Down Expand Up @@ -257,7 +257,12 @@ impl<'app> ProjectGraphBuilder<'app> {
for (task_id, task_config) in &project.tasks {
for task_dep in &task_config.deps {
if let TargetScope::Project(dep_id) = &task_dep.scope {
if project.dependencies.contains_key(dep_id) || dep_id == &id {
if
// Already a dependency
project.dependencies.contains_key(dep_id) ||
// Don't reference itself
project.matches_locator(dep_id.as_str())
{
continue;
}

Expand Down Expand Up @@ -339,17 +344,16 @@ impl<'app> ProjectGraphBuilder<'app> {
builder.extend_with_task(task_id, task_config);
}

let mut project = builder.build().await?;

// Inherit alias (is there a better way to do this?)
// Inherit alias before building incase the project
// references itself in tasks or dependencies
for (alias, project_id) in &self.aliases {
if project_id == id {
project.alias = Some(alias.to_owned());
builder.set_alias(alias);
break;
}
}

Ok(project)
builder.build().await
}

/// Enforce project constraints and boundaries after all nodes have been inserted.
Expand Down Expand Up @@ -489,10 +493,10 @@ impl<'app> ProjectGraphBuilder<'app> {
self.context.as_ref().unwrap()
}

fn resolve_id(&self, alias_or_id: &str) -> Id {
match self.aliases.get(alias_or_id) {
fn resolve_id(&self, project_locator: &str) -> Id {
match self.aliases.get(project_locator) {
Some(project_id) => project_id.to_owned(),
None => Id::raw(alias_or_id),
None => Id::raw(project_locator),
}
}
}
6 changes: 6 additions & 0 deletions nextgen/project/src/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,12 @@ impl Project {
.iter()
.any(|file| file.starts_with(&self.source))
}

/// Return true if the provided locator string (an ID or alias) matches the
/// current project.
pub fn matches_locator(&self, locator: &str) -> bool {
self.id.as_str() == locator || self.alias.as_ref().is_some_and(|alias| alias == locator)
}
}

impl Queryable for Project {
Expand Down
18 changes: 16 additions & 2 deletions nextgen/task-builder/src/tasks_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,11 @@ impl<'proj> TasksBuilder<'proj> {
}
}

trace!(id = self.project_id, "Filtering global tasks");
trace!(
id = self.project_id,
tasks = ?global_config.tasks.keys().map(|k| k.as_str()).collect::<Vec<_>>(),
"Filtering global tasks",
);

for (task_id, task_config) in &global_config.tasks {
let target = Target::new(self.project_id, task_id).unwrap();
Expand Down Expand Up @@ -190,6 +194,12 @@ impl<'proj> TasksBuilder<'proj> {
self.project_env.insert(key, value);
}

trace!(
id = self.project_id,
tasks = ?local_config.tasks.keys().map(|k| k.as_str()).collect::<Vec<_>>(),
"Loading local tasks",
);

self.local_tasks.extend(&local_config.tasks);

for id in local_config.tasks.keys() {
Expand All @@ -213,7 +223,11 @@ impl<'proj> TasksBuilder<'proj> {
async fn build_task(&self, id: &Id) -> miette::Result<Task> {
let target = Target::new(self.project_id, id)?;

trace!(target = target.as_str(), "Building task");
trace!(
target = target.as_str(),
"Building task {}",
color::id(id.as_str())
);

let mut task = Task::default();
let chain = self.get_config_inherit_chain(id);
Expand Down
2 changes: 2 additions & 0 deletions packages/cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
- Fixed an issue where non-YAML files in `.moon/tasks` would be parsed as YAML configs.
- Fixed an issue where arguments were not passed to generated Git hooks.
- Fixed an issue where moonbase would fail to sign in in CI.
- Fixed an issue where a root project with aliases, that has self referential tasks, would trigger a
stack overflow error.

## 1.14.1

Expand Down
3 changes: 2 additions & 1 deletion website/docs/commands/query/projects.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ $ moon query touched-files | moon query projects --affected

### Options

- `--affected` - Filter projects that have been affected by touched files.
- `--affected` - Filter projects that have been affected by touched files. This will only filter
based on files, and _does not_ include upstream or downstream dependencies.
- `--json` - Display the projects in JSON format.

#### Filters
Expand Down
3 changes: 2 additions & 1 deletion website/docs/commands/query/tasks.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ passing the `--json` flag. The output has the following structure:

### Options

- `--affected` - Filter projects that have been affected by touched files.
- `--affected` - Filter projects that have been affected by touched files. This will only filter
based on files, and _does not_ include upstream or downstream dependencies.
- `--json` - Display the projects in JSON format.

#### Filters
Expand Down