-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
make build names much smaller #2589
Conversation
Should fix windows path name limitations in most cases (unless your package name is very long)
@ibuclaw fixes the issue you commented about |
When I first did the change I considered reducing this as well. At the time I didn't envision the Windows path issue, so the tradeoff didn't seem worth it. How long do we have to get this in @ibuclaw ? |
other advantages than windows path issues:
disadvantages of this change:
@atilaneves are you using these path based informations somewhere in your DUB wrappers perhaps? |
@Geod24 if you want this in the next point release, until the 15th. I've just tagged the beta now. :-) |
From the top of my head, what I found most useful was to know which version of the compiler a project was compiled with. |
FAOD, the 2.103 stable cycle begins on 1st March, so if you'd rather defer to next major release, that's fine as well - there's absolutely no pressure to rush things into 2.102 minor releases if they aren't necessary. |
I would suggest we could wait with this for the next minor release, not a patch release. It's not a terribly important feature and it's a new feature, so it shouldn't actually really go into a patch release. |
No, reggae does its own thing. |
ok let's merge this then |
@WebFreak001 : Thanks for fixing this. We have plenty of evidence that it triggers too often for user, so glad to see it's going to be in the next release. |
Yeppa, thanks a lot! |
settings.platform.platform.join("."), | ||
settings.platform.architecture.join("."), | ||
settings.platform.compiler, settings.platform.compilerVersion, hashstr); | ||
return format("%s-%s-%s", config, settings.buildType, hashstr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside - why is buildType
there? I'm especially wondering about unittest
- AFAIK, that only really applies to the main package. But all dependencies then get unittest
too, although they aren't compiled with -unittest
etc., so might likely have to be recompiled even if a suitable regular debug
build might be in the build cache already.
Should fix windows path name limitations in most cases (unless your package name is very long)
Should improve verbose log output / the linker line in regular output + debugging paths in broken builds and compiler invocations.
In the unittest we just pick a smaller name to fix the windows test runner.
Moved platforms (linux.posix) + architecture (x86_64) + compiler (dmd_v2.102.0) out of the filename into the hash. While they may be interesting, I think, especially architecture and such, are rather internal. So I decided to un-expose those in the filenames.
From the sha256 we only take half the hash now (should be plenty of bytes still) + instead of hex strings, which are 200% of byte size, we use base64 (url, with padding removed), which is only 133% of byte size.
Sample in naming changes:
@Geod24 do you think this is a breaking change? I think at least the checksum could be halfed in length without incompatibilities.