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

Add a hidden option to store url as hash. #1794

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gmierz
Copy link
Collaborator

@gmierz gmierz commented May 20, 2022

This patch adds an option to use the hash of the url as the folder path instead of the url itself. This helps with an issue we are having on Windows where the path-character limit is being exceeded and the work-arounds offered by Microsoft don't fix it.

@gmierz gmierz requested a review from soulgalore May 20, 2022 11:47
@gmierz
Copy link
Collaborator Author

gmierz commented May 20, 2022

@soulgalore let me know what you think of this. We weren't able to get around the issue with the path limit in any other way. I've left the option hidden (like other options in this file) unless you'd like me to expose it.

@soulgalore
Copy link
Member

Wanted to check that you tried actions/runner-images#1052 (comment) with Set-itemproperty instead? I was thinking I wanted to add a test case with a long URL in the GitHub action but I guess its enabled by default on GitHub.

@gmierz
Copy link
Collaborator Author

gmierz commented May 20, 2022

Yes, the developer that had the problem tried it and it didn't work for them (they are on a windows VM as well). The other issue here is that we can't ask all of our developers to start playing with registry entries to run performance tests.

@soulgalore are you concerned about making this change?

@soulgalore
Copy link
Member

Yes, the developer that had the problem tried it and it didn't work for them (they are on a windows VM as well). The other issue here is that we can't ask all of our developers to start playing with registry entries to run performance tests.

Yes I understand but I wonder if they do not have hit the 260 limit in other cases too? I haven't been developing on Windows forf 15 years though so I don't know :) It seems like Python has administration rights and can set that property but maybe it's an evil thing to just change.

@soulgalore are you concerned about making this change?

The file generation is delicate and the exact code is duplicated in sitespeed.io. It's been something I been wanting to change for a while to make it cleaner/easier to understand, but it means work and a lot of testing and I want that to happen in a major release. We have --storeURLsAsFlatPageOnDisk that has the limit of 255 characters, but then there's a risk of colliding URLs. I was thinking of hacking something just for Windows but maybe the hash way is the way to go, let me just think about it over the weekend?

@gmierz
Copy link
Collaborator Author

gmierz commented May 21, 2022

Oh I see, that sounds good to me. Take your time, there's no rush since the dev can easily apply my patch as a fix right now.

@soulgalore
Copy link
Member

soulgalore commented May 22, 2022

@gmierz how do you feel about this idea:
We try to know if long paths is enabled on Windows, and if it is we just continue as before. If its not, we switch to your hash-solution (for all Windows machines without that setting), we do a info log that since you run on Windows and has not enabled long paths, you will get hashed filenames (and a link or so how to change the setting)?

After some tries yesterday I think I got the logic to work how to know if long paths is enabled:
https://github.com/sitespeedio/browsertime/pull/1795/files#diff-5db63aa79736d95a3ee6a4819789210e375174413b97f21faddb44a4f39d60efR55-R67

That way we will know it will work on all Windows machines (without adding any cli parameter) and we also give the opportunity to the user to fix the issue?

Also noticed that we instantiate two storage managers, I will have a look at that first.

@soulgalore
Copy link
Member

@gmierz I can implement what I proposed in #1794 (comment) - just let me know that you also agree on it (when you have time!).

@gmierz
Copy link
Collaborator Author

gmierz commented Jun 2, 2022

@soulgalore that sounds excellent! I checked your patch there and it's much simpler than I thought it would be to get that info. If you have time, it would be much appreciated if you implement it (if not that's no problem, I'll get to it before the end of the month).

@soulgalore
Copy link
Member

Cool, I'll fix it this weekend then!

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

Successfully merging this pull request may close these issues.

3 participants