Skip to content

Adding a dev website path for the new lints.json format and testing deployment #7229

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

Closed
wants to merge 2 commits into from

Conversation

xFrednet
Copy link
Member

@xFrednet xFrednet commented May 16, 2021

A new PR to feed the beast. This one simply added a new dev path to GitHub Pages and automatically deploys the new dev-index.html as well as a lints.json file along it. The lints.json file for the dev environment will be created using our new metadata collection (monster).

The dev website is currently fairly broken and with that I mean: It doesn't display any lints at all. This is on purpose as this PR only aims to create the basic setup to make future reviews simpler. The adapting to the new lints.json format will be done on the dev-index.html to allow a preview while not impacting our main lint list.

The changes are split into two commits which will probably help with the review as the dev-index.html is just a straight copy from index.html and doesn't have to be reviewed.

This is also a test to see if the new deploy script works in the CI environment, It works locally so I have high hopes ^^


See: #7172 for the full metadata collection to-do list or to suggest a new feature in connection to it 🙃


changelog: none

r? @flip1995

This is the last branch I had in my backlog. The next one will take some time again xD.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 16, 2021
@xFrednet xFrednet changed the title 7172 feed the beast in ci Adding a dev website path for the new lints.json format and testing deployment May 16, 2021
@flip1995
Copy link
Member

It works locally so I have high hopes

Let me tell you that CI/CD has a talent for crushing those hopes.

The next one will take some time again

I wanted to jump in on the CI work anyway and take over a bit. When I first set up the CI, I didn't really document it well. Especially how things work together with the release process. There are some weirdnesses to consider.

I'm not sure if we should deploy the dev-static.html file yet. I want to avoid having to clean up the gh-pages branch. I'll have to think about what the best thing to do here is.

@xFrednet
Copy link
Member Author

xFrednet commented May 17, 2021

It works locally so I have high hopes

Let me tell you that CI/CD has a talent for crushing those hopes.

That made me laugh more than it should have ^^. My dreams are still alive until then.

I wanted to jump in on the CI work anyway and take over a bit. When I first set up the CI, I didn't really document it well. Especially how things work together with the release process. There are some weirdnesses to consider.

Awesome, working on CI/CD is not my favorite part of the development process anyways. So, feel free to take over. One thing to consider would be to adapt the build and testing steps in the pipeline to include the metadata-collector-lint feature. I've also noticed in that regard that the build step doesn't deny warnings from the compiler itself. I'm not sure if that is intentional or not. These would be caught by the dogfood test anyways.

Oh, and another thing. I wanted to merge the doc comment headline switch (**<headline>** -> ### <headline>) separately, so it would be cool if you could include the sed line when you're working on it. We could also switch to the new headline format beforehand, however I'm not sure if we want to change the python script anymore. What are your thoughts about it? On second thought I believe that it would actually be better to switch beforehand, the adapting in the python script should be minor, and it would simplify the first deployment.

# Temporary fix until the old headline syntax will be replaced with the markdown ### syntax
sed -i -e 's/ \*\*/### /g' -e 's/\*\*/\\n/g' util/gh-pages/metadata_collection.json

I'm not sure if we should deploy the dev-static.html file yet. I want to avoid having to clean up the gh-pages branch. I'll have to think about what the best thing to do here is.

I can easily deploy the new website to my repo for now and link to it for a preview. That wouldn't mess with the gh-pages branch and doesn't cost me anything. The drawbacks would be that you had to review all website changes at once and that the deployment wouldn't be tested beforehand, which is fine for me if you take that over 🙃.

@flip1995
Copy link
Member

Awesome, working on CI/CD is not my favorite part of the development process anyways.

Perfect, then we have a deal. For some reason I like the pain that comes with CI/CD.

I've also noticed in that regard that the build step doesn't deny warnings from the compiler itself. I'm not sure if that is intentional or not.

It should deny warnings. Maybe it doesn't deny them in the metadata collector lint, since that feature is not (and should not be) enabled during normal CI builds. Having those only caught in the dogfood test isn't ideal, but should be good enough for now.

I believe that it would actually be better to switch beforehand

Yes, me too. I'd like to get all of this merged in one sync / release cycle. So all the work that modifies CI should be batched into one day, where the tree is closed otherwise. This will reduce the impact that potentially breaking the deployment will have. From experience I would say that this has a high likelihood.

I can easily deploy the new website to my repo for now and link to it for a preview. [...] The drawbacks would be that you had to review all website changes at once and that the deployment wouldn't be tested beforehand

That would be better I think (together with my point above). You can just ping me on Zulip, if you have something ready to review or want feedback while working on it. I can then review it in your fork without a PR.

@xFrednet
Copy link
Member Author

For some reason I like the pain that comes with CI/CD.

I kind of want to quote you on that, after everything is merged ^^

It should deny warnings.

I encountered this case then I added the metadata-collection-lint feature to the build script in a test commit. See pipeline. It might be a different script as the one used in PRs as this one was triggered via a push.

So all the work that modifies CI should be batched into one day, where the tree is closed otherwise.

Awesome, I'll adapt the website to a new working state, and then we can try to find a date for this. Should I close this PR in the meantime?

You can just ping me on Zulip, if you have something ready to review or want feedback while working on it

👍

@flip1995
Copy link
Member

See pipeline.

Ah thanks, I know what's going on, thanks for pointing that out! I'll prepare a fix.

Should I close this PR in the meantime?

Yeah, I don't think we'll merge this, but split it up in multiple other PRs.

@flip1995 flip1995 closed this May 18, 2021
bors added a commit that referenced this pull request May 18, 2021
Deny warnings in every main sub-crate

Pointed out by `@xFrednet` in #7229 (comment)

This enables the same (rustc) lints in every main sub-crate:

- `clippy`
- `clippy_lints`
- `clippy_utils`
- `clippy_dev`

In addition it forwards the `deny-warnings` feature to those sub-crates, so we don't miss warnings that then become a problem during sync. (I wanted to fix that before, but forgot about it, so thanks for pointing it out `@xFrednet!)`

changelog: none
@xFrednet xFrednet deleted the 7172-feed-the-beast-in-ci branch August 18, 2021 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants