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

Remove -Xmax-classfile-length; hard-code to 240 #7497

Merged
merged 1 commit into from Jan 28, 2019

Conversation

@hrhino
Copy link
Member

@hrhino hrhino commented Dec 5, 2018

This option allowed users to limit the size of classfile names
to fewer than 255 (previously the default) characters.

However, this means that classfiles made from a compiler with the
option are not compatible with classfiles made without it
(in either direction), since the compiler will flatten names of
classes with the current value of the setting, not whatever the
class was compiled with.

So, remove it and default to 240 (the limit in docker).

See also scala/bug#8199

@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Dec 5, 2018
2 * (settings.maxClassfileName.value - maxSuffixLength - 2*marker.length - 32)
)
final val marker = "$$$$"
final val MaxSuffixLength = 7 // "$.class".length + 1 // potential module class suffix and file extension

This comment has been minimized.

@hrhino

hrhino Dec 5, 2018
Author Member

actually, this is different (8 -> 7). I can't tell why the +1 was in maxSuffixLength...

@SethTisue
Copy link
Member

@SethTisue SethTisue commented Dec 5, 2018

do we need to deprecate-then-remove?

@hrhino
Copy link
Member Author

@hrhino hrhino commented Dec 5, 2018

Your call. It can get deprecated in 2.12.9, right?

@SethTisue
Copy link
Member

@SethTisue SethTisue commented Dec 5, 2018

It can get deprecated in 2.12.9, right?

yes but it's a bit weird since very likely 2.12.9 won't come out until after 2.13.0-RC1. and there's no particular rush, right?

@lrytz
Copy link
Member

@lrytz lrytz commented Dec 5, 2018

I'd say the flag can go without depracation, given how -X and how useless it is :)

@SethTisue
Copy link
Member

@SethTisue SethTisue commented Dec 5, 2018

the only recent discussion in this area I can find is at #5088

@SethTisue
Copy link
Member

@SethTisue SethTisue commented Dec 5, 2018

also I closed scala/bug#3623, we'll see if that produces any reaction

@mkeskells
Copy link
Contributor

@mkeskells mkeskells commented Dec 5, 2018

For our use cases changing the behavior is fine (and I would be happy to have this in 2.12.x) .

In general it breaks binary compatability though doesn't it (e.g. serialisation). It seems that it was already broken and this is stability though so it seems better that we had before

@hrhino
Copy link
Member Author

@hrhino hrhino commented Dec 5, 2018

Changing it breaks binary/serialisation compat, yes. This is 2.13 so neither of those are guaranteed anyways. If we did the other (more complicated) solution of pickling flattened names it would allow artifacts produced with one value to be used to compile sources even with a different value; this might be helpful to e.g. the ecryptfs people.

@smarter
Copy link
Contributor

@smarter smarter commented Dec 5, 2018

People on #5088 seem to report that 240 might be a better default limit than 255, something to consider if we decide on hardcoding it.

@lrytz
Copy link
Member

@lrytz lrytz commented Dec 12, 2018

I'd go for 240 then if that helps running in docker (https://stackoverflow.com/a/42840554/248998)

@smarter
Copy link
Contributor

@smarter smarter commented Dec 12, 2018

Though note that ecryptfs developers recommend using a limit of 140 characters (https://unix.stackexchange.com/a/32834) and I think that on Ubuntu, /home is encrypted with ecryptfs by default.

@lrytz
Copy link
Member

@lrytz lrytz commented Dec 12, 2018

I wonder how common ecryptfs is. Did we ever get reports of people being affected by this? It also seems they removed ecryptfs from ubuntu in 18.04 (https://www.linuxuprising.com/2018/04/how-to-encrypt-home-folder-in-ubuntu.html). Is full-disk encryption affected?

@Atry
Copy link
Contributor

@Atry Atry commented Jan 10, 2019

I think limiting only names of private classes will not break compatibility.

This option allowed users to limit the size of classfile names
to fewer than 255 (previously the default) characters.

However, this means that classfiles made from a compiler with the
option are not compatible with classfiles made without it
(in either direction), since the compiler will flatten names of
classes with the current value of the setting, not whatever the
class was compiled with.

So, remove it.
@adriaanm adriaanm force-pushed the hrhino:kill/Xmax-classfile-name branch from db2148c to a8fa7e6 Jan 24, 2019
@adriaanm adriaanm changed the title Remove -Xmax-classfile-length Remove -Xmax-classfile-length; hard-code to 240. Jan 24, 2019
@adriaanm

This comment has been hidden.

@adriaanm

This comment has been hidden.

@adriaanm
Copy link
Member

@adriaanm adriaanm commented Jan 28, 2019

I took a look at how popular ecryptfs might be, and I conclude we shouldn't worry too much. https://askubuntu.com/questions/1029249/how-to-encrypt-home-on-ubuntu-18-04

@adriaanm adriaanm merged commit 60b673e into scala:2.13.x Jan 28, 2019
3 checks passed
3 checks passed
@scala-jenkins
cla @hrhino signed the Scala CLA. Thanks!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@scala-jenkins
validate-main [6810] SUCCESS. Took 58 min.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
8 participants