Validate triagebot.toml in tidy/CI

[compiler-errors]

Would've prevented #106102, which broke in #105661.

We should at least validate the toml file is valid syntax, but ideally we'd validate more context-specific stuff.

[jyn514]

Mentoring instructions: Add a new check to tidy around here that parses triagebot.toml to make sure it's valid toml:

rust/src/tools/tidy/src/lib.rs

Lines 41 to 58 in 17395b4

	pub mod alphabetical;
	pub mod bins;
	pub mod debug_artifacts;
	pub mod deps;
	pub mod edition;
	pub mod error_codes_check;
	pub mod errors;
	pub mod extdeps;
	pub mod features;
	pub mod mir_opt_tests;
	pub mod pal;
	pub mod primitive_docs;
	pub mod style;
	pub mod target_specific_tests;
	pub mod ui_tests;
	pub mod unit_tests;
	pub mod unstable_book;
	pub mod walk;

See the existing tidy files for an example of how to write a tidy check.

[aadityadhruv]

Hi, I'm a new contributor here and thought of picking this as my first issue. From what I understand, a new crate (something like triagebot.rs) needs to be made for this issue right? Where we can parse the toml and see if it is valid.

[jyn514]

@aadityadhruv a new module, not a new crate. The module should be part of tidy so check runs on x test tidy.

[aadityadhruv]

Right, that makes sense. Thank you!

[aadityadhruv]

Hi, sorry for the late response, I was out traveling. I'm currently confused about a certain thing. Currently what I am trying to do is manually check the file for valid syntax, by going over it line by line. Is this the correct method to do this, or should I be using an already existing crate like toml? I would assume that extra crate dependencies aren't probably wanted.

Also, I was wondering something about Path in the check function. For this module, would I need to check/filter any paths like some other modules do? I was assuming that I will simply open path + 'tiragebot.toml' and read that file.

Any help is appreciated!

[jyn514]

should I be using an already existing crate like toml?

Yes, please use toml, don't use regex or anything like that.

[ehuss]

I personally would prefer to see this implemented in triagebot itself instead of tidy. That would make it work for all repositories. It should be fairly trivial to add a handler that checks for a PR that modifies triagebot.toml and to validate it, and post a comment if it is not correct. It can even use the Config type to verify that the actual structure matches. Bonus points would to also use serde-ignored to enforce that unknown fields aren't included.

[dotdot0]

@aadityadhruv are you still working on this?

[aadityadhruv]

Hi, yeah I'm sorry, I'm still working on it. I've been pretty busy the last couple of days, I'll get it done this week itself. Hope that's ok!

[dotdot0]

Hi, yeah I'm sorry, I'm still working on it. I've been pretty busy the last couple of days, I'll get it done this week itself. Hope that's ok!

Yeah! it's alright

[aadityadhruv]

I personally would prefer to see this implemented in triagebot itself instead of tidy. That would make it work for all repositories. It should be fairly trivial to add a handler that checks for a PR that modifies triagebot.toml and to validate it, and post a comment if it is not correct. It can even use the Config type to verify that the actual structure matches. Bonus points would to also use serde-ignored to enforce that unknown fields aren't included.

Sorry for being slow with the issue, but along with being busy, I've been kinda stuck, and was unsure if I should ask for help. I am currently unsure if I should be doing work in tidy or the triagebot repository. Should I first try to get the tidy check working and then maybe open an issue on the triagebot repo?

[aadityadhruv]

Here is the code I had written (it does not work since I do not know what the configuration of the toml is):

use crate::walk::{filter_dirs, walk};
use serde::Deserialize;
use std::path::Path;

#[derive(Deserialize)]
struct Config {}

pub fn check(path: &Path, bad: &mut bool) {
    walk(path, &mut |path| filter_dirs(path), &mut |entry, contents| {
        let file = entry.path();
        let filename = file.file_name().unwrap();
        if filename != "triagebot.toml" {
            return;
        }
        let conf: Result<Config, toml::de::Error> = toml::from_str(contents);
        match conf {
            Ok(_) => {}
            Err(_err) => {
                tidy_error!(bad, "{} is an invalid toml file", file.display())
            }
        }
    });
}

I am unsure what would be the right next step here.

[madsravn]

Hi @aadityadhruv . I am new here and just looking through for an issue I can get started with. So keeping that in mind, here is my idea.

If you look at the documentation for the toml crate there is a method where you can just parse an entire string into a toml::Value. This Value is an enum for the entire toml structure. My idea is that if you can parse the triagebot.toml it is correct toml and you have validated the file.

My very short code is here:

use toml::Value;

fn main() {
    let input = std::fs::read_to_string("triagebot.toml").unwrap();
    let value = input.parse::<Value>().unwrap();
    println!("{value}");
}

I hope it helps.

[aadityadhruv]

This looks great! Thank you!

[aadityadhruv]

Here is a PR for the same: #106559

[jyn514]

I personally would prefer to see this implemented in triagebot itself instead of tidy. That would make it work for all repositories. It should be fairly trivial to add a handler that checks for a PR that modifies triagebot.toml and to validate it, and post a comment if it is not correct. It can even use the Config type to verify that the actual structure matches. Bonus points would to also use serde-ignored to enforce that unknown fields aren't included.

👍 #106559 (comment)

[BoxyUwU]

Just thought I'd drop by and say that this would have been nice to avoid #106848 😅

[jyn514]

Ah, that wouldn't have been caught actually - the toml syntax was valid, the file just didn't exist in the repository. Checking if the glob has at least one match seems possible.

[0xJepsen]

I see the PR up but not sure if anyone is still working on the requested changes?

[aadityadhruv]

Yeah I'm currently working on it in the triagebot repo, I haven't opened a PR/Issue yet for it.

[meysam81]

@rustbot claim

[meysam81]

Can someone please review this change: rust-lang/triagebot#1730