browsertime icon indicating copy to clipboard operation
browsertime copied to clipboard

Add a hidden option to store url as hash.

Open gmierz opened this issue 3 years ago • 9 comments

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 avatar May 20 '22 11:05 gmierz

@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.

gmierz avatar May 20 '22 11:05 gmierz

Wanted to check that you tried https://github.com/actions/virtual-environments/issues/1052#issuecomment-644212560 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.

soulgalore avatar May 20 '22 14:05 soulgalore

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?

gmierz avatar May 20 '22 16:05 gmierz

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?

soulgalore avatar May 20 '22 20:05 soulgalore

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.

gmierz avatar May 21 '22 02:05 gmierz

@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 avatar May 22 '22 09:05 soulgalore

@gmierz I can implement what I proposed in https://github.com/sitespeedio/browsertime/pull/1794#issuecomment-1133855731 - just let me know that you also agree on it (when you have time!).

soulgalore avatar May 30 '22 19:05 soulgalore

@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).

gmierz avatar Jun 02 '22 15:06 gmierz

Cool, I'll fix it this weekend then!

soulgalore avatar Jun 02 '22 19:06 soulgalore