submodule detection for proper fix on #96188

[onur-ozkan]

This commit resolves an internal FIXME note within the bootstrap by implementing submodule detection. This is accomplished through an iterative process over the .gitmodules file.

r? @albertlarsan68

[albertlarsan68]

I feel like the approach to get submodules is good, but I don't think the code does the wanted thing. I haven't tested my theory, but I feel like you should cache the submodules list. This avoids a problem (I think) where the first path goes until the end of the file, and the second path does nothing, because the cursor is already at the end, and is not rewound. The other option would be to rewind the file, but it would be less efficient.

[onur-ozkan]

I feel like the approach to get submodules is good, but I don't think the code does the wanted thing. I haven't tested my theory, but I feel like you should cache the submodules list. This avoids a problem (I think) where the first path goes until the end of the file, and the second path does nothing, because the cursor is already at the end, and is not rewound. The other option would be to rewind the file, but it would be less efficient.

File is 34 lines long :D And I don't think it will ever reach more than ~70. Doing proper caching will require keeping those values in the parent configuration within HashMap for O(1) access. Wouldn't that be overkill for this particular purpose? I would prefer forgetting the values once we finish the process here rather than keeping it in memory permamently.

[onur-ozkan]

Doing proper caching will require keeping those values in the parent configuration within HashMap for O(1) access.

Just remember now, and even with HashMap O(1) will not be possible because we don't have the complete values https://github.com/rust-lang/rust/blob/0c95fc504e881c796bd1a042b72b1d4c445c47a5/src/bootstrap/builder.rs#L501

so we will need to iterate the values no mather what

[albertlarsan68]

A HashMap is definitely overkill, I was thinking about keeping the Vec that you build.

[onur-ozkan]

A HashMap is definitely overkill, I was thinking about keeping the Vec that you build.

We can do that by adding it to Config, or require extra arg for paths function as Vec for submodules(which will require every caller calculating submodules first).

[onur-ozkan]

Now it should be much better @albertlarsan68

[albertlarsan68]

There is already once_cell in bootstrap, can you use it and add a FIXME to use std when stabilized?
Otherwise good.

[onur-ozkan]

add a FIXME to use std when stabilized?

This isn't needed, I updated the existing FIXME which aims the same purpose.

[albertlarsan68]

r=me with the unwrapchanged to expect

[onur-ozkan]

@bors r=albertlarsan68 rollup

[bors]

📌 Commit 5a4066e has been approved by albertlarsan68

It is now in the queue for this repository.