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

make build names much smaller #2589

Merged
merged 2 commits into from
Feb 25, 2023
Merged

Conversation

WebFreak001
Copy link
Member

@WebFreak001 WebFreak001 commented Feb 12, 2023

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:

/home/webfreak/.dub/cache/PACKAGE_NAME/~master/build/application-debug-linux.posix-x86_64-dmd_v2.102.0-D95E91464418B72BDBF518DAA90BF13D4018E81A07B52C64F0C5EB6A60977D58/TARGET_NAME
->
/home/webfreak/.dub/cache/PACKAGE_NAME/~master/build/application-debug-aCpueoVTBosJsJSHQ-aldQ/TARGET_NAME

@Geod24 do you think this is a breaking change? I think at least the checksum could be halfed in length without incompatibilities.

Should fix windows path name limitations in most cases (unless your
package name is very long)
@WebFreak001
Copy link
Member Author

@ibuclaw fixes the issue you commented about

@Geod24
Copy link
Member

Geod24 commented Feb 12, 2023

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 ?

@WebFreak001
Copy link
Member Author

other advantages than windows path issues:

  • verbose log is more readable
  • compiler invocations are more readable

disadvantages of this change:

  • can't look into internal build files to see which compiler was used, etc.

@atilaneves are you using these path based informations somewhere in your DUB wrappers perhaps?

@ibuclaw
Copy link
Member

ibuclaw commented Feb 12, 2023

@Geod24 if you want this in the next point release, until the 15th. I've just tagged the beta now. :-)

@Geod24
Copy link
Member

Geod24 commented Feb 12, 2023

From the top of my head, what I found most useful was to know which version of the compiler a project was compiled with.
It becomes less important with a GC for the cache (and I just realize that GCing the cache might need to use those informations).

@ibuclaw
Copy link
Member

ibuclaw commented Feb 12, 2023

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.

@WebFreak001
Copy link
Member Author

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.

@WebFreak001
Copy link
Member Author

WebFreak001 commented Feb 13, 2023

will fix #911, fix #2538, fix #2443

@atilaneves
Copy link
Contributor

No, reggae does its own thing.

@WebFreak001
Copy link
Member Author

ok let's merge this then

@Geod24
Copy link
Member

Geod24 commented Mar 1, 2023

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

@kinke
Copy link
Contributor

kinke commented Mar 11, 2023

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);
Copy link
Contributor

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.

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.

5 participants