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

Remove DocBuilder, part 1 #1016

Merged
merged 8 commits into from
Aug 30, 2020
Merged

Conversation

pietroalbini
Copy link
Member

This PR is the first of the two PRs I plan to send to remove DocBuilder and DocBuilderOptions from the docs.rs codebase: both those structs serve as "intermediaries" between RustwideBuilder and the rest of the application, but now that we have Config and Context that's not really needed anymore. Not having the middle layer will help with building buildtests a lot.

Notable changes:

  • cratesfyi build --skip-if-log-exists <command> was removed: the --skip-if-log-exists flag was really fragile, relying on a text log of all the builds updated every 10 builds. There is also no need I can see for it, as the practically equivalent (and better) --skip-if-exists already exists.
  • cratesfyi build --skip-if-exists <command> was improved: now it checks if a successful build is present, instead of checking whether the release metadata is stored in the database.
  • All the RustwideBuilder configuration was moved over to Config.
  • All of DocBuilderOptions was moved over to Config.
  • The Index is now stored in Context and a single instance is reused across the whole application. This won't affect the file descriptors problem, as the underlying git instance is still re-created every time it is accessed.

In the next PR I'll move away all the rest of DocBuilder. This PR is best reviewed commit-by-commit.

Those options aren't used at all anywhere in the codebase, so it's safe
to remove them.
As far as I'm aware this flag was never used since the Rust project
started to take care of docs.rs, and it's not much useful.

Basically, every time a build happened the DocBuilder recorded the crate
name and version in a text file, and the flag prevented a build if the
related names and versions were present in such file. Checking if the
build is successful in the database is more reliable.
@jyn514 jyn514 added A-builds Area: Building the documentation for a crate S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Aug 29, 2020
@@ -48,21 +48,13 @@ pub fn queue_builder(

if status.count() >= 10 {
// periodically, we need to flush our caches and ping the hubs
debug!("10 builds in a row; flushing caches");
debug!("10 builds in a row; pinging pubsubhubhub");
status = BuilderState::QueueInProgress(0);

match pubsubhubbub::ping_hubs() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know we had this 😆 do we know anyone using this? I'd rather have #809 I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno why it exists, but let's not remove it for now until we get more insights on who's using it.

src/utils/queue_builder.rs Outdated Show resolved Hide resolved
Comment on lines +664 to +665
"SELECT 1 FROM crates, releases, builds
WHERE crates.id = releases.crate_id AND releases.id = builds.rid
Copy link
Member

@jyn514 jyn514 Aug 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs an INNER JOIN or you'll get duplicates (the id won't match the crate that succeeded).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm? This is working as expected when testing manually. Why would you expect it to fail?

Copy link
Member

@jyn514 jyn514 Aug 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I expected it to fail when you have three different crates, each of which satisfies one of these conditions.

CREATE TEMPORARY TABLE crates (
  id SERIAL PRIMARY KEY,
  name VARCHAR(255) UNIQUE NOT NULL
);

CREATE TEMPORARY TABLE releases (
   id SERIAL PRIMARY KEY,
   crate_id INT NOT NULL REFERENCES crates(id),
   version VARCHAR(100)
);

CREATE TEMPORARY TABLE builds (
   id SERIAL,
   rid INT NOT NULL REFERENCES releases(id),
   build_status BOOL NOT NULL
);

insert into crates (name) values ('a'), ('b');
insert into releases (crate_id, version) values (1, '0.1.0'), (2, '0.1.0');
insert into builds (rid, build_status) values (1, true), (2, true);

SELECT 1 FROM crates, releases, builds
WHERE crates.id = releases.crate_id AND releases.id = builds.rid
  AND crates.name = 'a' AND releases.version = '0.1.0'  AND builds.build_status = TRUE;

But that doesn't happen - the WHERE id = crate_id AND id = rid filters out the duplicates. It's more clear when you select everything instead of 1:

select * from crates, releases, builds WHERE crates.id = releases.crate_id AND releases.id = builds.rid;
 id | name | id | crate_id | version | id | rid | build_status 
----+------+----+----------+---------+----+-----+--------------
  1 | a    |  1 |        1 | 0.1.0   |  1 |   1 | f
  2 | b    |  2 |        2 | 0.1.0   |  2 |   2 | t
joshua=> select * from crates, releases, builds WHERE crates.name = 'a' AND releases.version = '0.1.0'  AND builds.build_status = TRUE;
 id | name | id | crate_id | version | id | rid | build_status 
----+------+----+----------+---------+----+-----+--------------
  1 | a    |  1 |        1 | 0.1.0   |  2 |   2 | t
  1 | a    |  2 |        2 | 0.1.0   |  2 |   2 | t
joshua=> select * from crates, releases, builds WHERE crates.name = 'a' AND releases.version = '0.1.0'  AND builds.build_status = TRUE AND crates.id = releases.crate_id AND releases.id = builds.rid;
 id | name | id | crate_id | version | id | rid | build_status 
----+------+----+----------+---------+----+-----+--------------
(0 rows)

So this works, it's just confusing.

src/config.rs Show resolved Hide resolved
src/docbuilder/rustwide_builder.rs Show resolved Hide resolved
src/bin/cratesfyi.rs Outdated Show resolved Hide resolved
src/utils/daemon.rs Show resolved Hide resolved
Co-authored-by: Joshua Nelson <joshua@yottadb.com>
@jyn514 jyn514 added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Aug 30, 2020
@jyn514 jyn514 merged commit e1ae6d9 into rust-lang:master Aug 30, 2020
@pietroalbini pietroalbini deleted the no-more-docbuilder-1 branch August 30, 2020 16:57
@jyn514 jyn514 removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Sep 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-builds Area: Building the documentation for a crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants