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

Clean build directory before preprocessors run instead of after. #1235

Closed
XAMPPRocky opened this issue May 23, 2020 · 8 comments
Closed

Clean build directory before preprocessors run instead of after. #1235

XAMPPRocky opened this issue May 23, 2020 · 8 comments

Comments

@XAMPPRocky
Copy link
Member

XAMPPRocky commented May 23, 2020

The change shown below introduced a regression in the https://forge.rust-lang.org where the our precessor can no longer generate HTML redirects that we need to maintain compatibility. (rust-lang/rust-forge#336). I would propose to perform the clean step before preprocessors are run.

if destination.exists() {
utils::fs::remove_dir_content(destination)
.with_context(|| "Unable to remove stale HTML output")?;
}

@ehuss
Copy link
Contributor

ehuss commented May 23, 2020

I think this is a duplicate of #1087. It was intentionally changed, and there is some discussion there of possible solutions. It would be nice to have someone would make a proposal. I've only skimmed it, but it looks like there's an interesting discussion of allowing preprocessors to add "resources", which I think would also solve #1222.

@XAMPPRocky
Copy link
Member Author

XAMPPRocky commented May 23, 2020

Well I think it should be reverted until there is a solution, otherwise I'll have to pin or maintain a fork of mdbook. It's not a great experience to intentionally break in a patch release.

@ehuss
Copy link
Contributor

ehuss commented May 23, 2020

I appreciate that it is frustrating when something breaks, but it has been almost 10 months since the change, it would also cause breakage to revert it. I think a more constructive discussion on #1087 would be more fruitful, and provide solutions for everyone. I suspect it is not too difficult to come up with a design for preprocessors to indicate additional resources they want to include. I encourage you (or anyone) to work with @Michael-F-Bryan to come up with some solution, I'd be happy to review.

Alternatively, if it is just redirect support, I would be happy to have a first-class solution for that (#430). I know it has come up many times, and would be very valuable. I'd by happy to discuss that, as well.

Closing as a duplicate of #1087.

@ehuss ehuss closed this as completed May 23, 2020
@XAMPPRocky
Copy link
Member Author

XAMPPRocky commented May 23, 2020

I appreciate that it is frustrating when something breaks, but it has been almost 10 months since the change, it would also cause breakage to revert it.

My problem is not the time scale, it's the fact that this project has no respect for semver, this isn't the first time patch releases have broken forge.

@Dylan-DPC-zz
Copy link

Breaking compatibility accidentally doesn't mean we are not respecting semver

@XAMPPRocky
Copy link
Member Author

I think this is a duplicate of #1087. It was intentionally changed

@Michael-F-Bryan
Copy link
Contributor

Michael-F-Bryan commented May 24, 2020

@XAMPPRocky, how are you generating the redirects? You mention before/after preprocessors, so it sounds like you've written your own Preprocessor to insert redirects?

The intent behind a Preprocessor is to modify the Book after it has been loaded into memory, and return the modified Book so it can be rendered by a particular backend (e.g. the HTML renderer).

Given that design, when a renderer chooses to clear its build directory shouldn't matter, because Preprocessors shouldn't know it exists and the directory is "owned" by the renderer.


Alternatively, if it is just redirect support, I would be happy to have a first-class solution for that (#430). I know it has come up many times, and would be very valuable. I'd by happy to discuss that, as well.

I think the best solution would be to incorporate redirects directly into the HTML renderer (as discussed in #430) instead of having to write your own preprocessor. @XAMPPRocky, is that something you would like us to look into implementing?

I'll be free tomorrow and the day after so should have plenty of time to implement HTML redirects as a first-class feature.

@XAMPPRocky
Copy link
Member Author

I've linked how forge generates redirects in its pre-processor below. I'm not interested in collaborating on a solution, breaking my code has already cost me a good chunk of time and stress dealing with people complaining that forge no longer works.

https://github.com/rust-lang/rust-forge/blob/master/blacksmith/src/lib.rs#L131

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants