Oddness regarding file locks in package.bbclass
I just stumbled upon something odd in package.bbclass. In commit ede381d5 from January 2011 (the code hasn't changed since), the use of the ${PACKAGELOCK} lock file was changed to shared to improve parallelism. However, when looking at the actual change it becomes confusing. I have included it below for reference.
Quoted 13 lines
commit
ede381d56b180b384fdad98d445e5430819cfadeAuthor: Richard Purdie <[email protected]> Date: Wed Jan 19 11:04:15 2011 +0000package.bbclass: Take a shared lock when reading to improve
do_packageparallelismSigned-off-by Richard Purdie <[email protected]>meta/classes/package.bbclass +0
-0
meta/classes/package.bbclass python emit_pkgdata() { · line 501
Quoted 6 lines
meta/classes/package.bbclass +2
-1@@ -497,7 +497,8 @@ python emit_pkgdata() { pkgdest = bb.data.getVar('PKGDEST', d, 1) pkgdatadir = bb.data.getVar('PKGDESTWORK', d, True) - lf = bb.utils.lockfile(bb.data.expand("${PACKAGELOCK}", d)) + # Take shared lock since we're only reading, not writing + lf = bb.utils.lockfile(bb.data.expand("${PACKAGELOCK}", d), True)
Here the lock is changed to shared as per the commit message.
meta/classes/package.bbclass python package_do_shlibs() { · line 654
Quoted 9 lines
meta/classes/package.bbclass +1
-0data_file = pkgdatadir + bb.data.expand("/${PN}" , d) f = open(data_file, 'w')@@ -649,6 +650,7 @@ python package_do_shlibs() { shlibs_dir = bb.data.getVar('SHLIBSDIR', d, True) shlibswork_dir = bb.data.getVar('SHLIBSWORKDIR', d, True) + # Take shared lock since we're only reading, not writing lf = bb.utils.lockfile(bb.data.expand("${PACKAGELOCK}", d))
Here, however, it is not changed, even though a comment is added to say that it is. Was this intentional, or just an oversight?
meta/classes/package.bbclass python package_do_pkgconfig () { · line 884
Quoted 8 lines
meta/classes/package.bbclass +1
-0def linux_so(root, path, file):@@ -878,6 +880,7 @@ python package_do_pkgconfig () { if hdr == 'Requires': pkgconfig_needed[pkg] += exp.replace(',', ' ').split() + # Take shared lock since we're only reading, not writing lf = bb.utils.lockfile(bb.data.expand("${PACKAGELOCK}", d))
Here again a comment is added, but the code is not changed to match.
meta/classes/package.bbclass python package_do_pkgconfig () { · line 885
Quoted 2 lines
meta/classes/package.bbclass +0
-0for pkg in packages.split():
Also, what is the ${PACKAGELOCK} lock file actually protecting? With the exception of the two questionable cases above, I cannot see that the lock is taken privately anywhere else. And since it looks as the code in package_do_shlibs() and package_do_pkgconfig() is not what needs protection (based on the added comments above), what is?
//Peter
Quoted 10 lines · Peter Kjellerstedt
Quoted 2 lines
+ # Take shared lock since we're only reading, not writing
lf = bb.utils.lockfile(bb.data.expand("${PACKAGELOCK}", d))Here, however, it is not changed, even though a comment is added to say that it is. Was this intentional, or just an oversight?
Quoted 2 lines
+ # Take shared lock since we're only reading, not writing
lf = bb.utils.lockfile(bb.data.expand("${PACKAGELOCK}", d))Here again a comment is added, but the code is not changed to match.
I'm no expert on this piece of code but that definitely looks like an oversight.
Quoted 2 lines
Also, what is the ${PACKAGELOCK} lock file actually protecting? With
the exception of the two questionable cases above, I cannot see that
Quoted 4 lines
the lock is taken privately anywhere else. And since it looks as the code in
package_do_shlibs()andpackage_do_pkgconfig()is not what needs protection (based on the added comments above), what is?
Good point. The sstate locks are also shared.
Richard was involved in all of these changes so lets wait for him to return from his holiday...
Ross
This item doesn't appear to have a follow-up.
Quoted 42 lines · Ross
Quoted 10 lines · Peter Kjellerstedt
Quoted 2 lines
+ # Take shared lock since we're only reading, not writing
lf = bb.utils.lockfile(bb.data.expand("${PACKAGELOCK}", d))Here, however, it is not changed, even though a comment is added to say that it is. Was this intentional, or just an oversight?
Quoted 2 lines
+ # Take shared lock since we're only reading, not writing
lf = bb.utils.lockfile(bb.data.expand("${PACKAGELOCK}", d))Here again a comment is added, but the code is not changed to match.
I'm no expert on this piece of code but that definitely looks like an oversight.
Quoted 2 lines
Also, what is the ${PACKAGELOCK} lock file actually protecting? With
the exception of the two questionable cases above, I cannot see that
Quoted 4 lines
the lock is taken privately anywhere else. And since it looks as the code in
package_do_shlibs()andpackage_do_pkgconfig()is not what needs protection (based on the added comments above), what is?Good point. The sstate locks are also shared.
Richard was involved in all of these changes so lets wait for him to return from his holiday...
Ross
--
_______________________________________________
Openembedded-core mailing list
[email protected]
http://lists.openembedded.org/mailman/listinfo/openembedded-core
Quoted 62 lines · Peter Kjellerstedt
I just stumbled upon something odd in package.bbclass. In commit
ede381d5from January 2011 (the code hasn't changed since), the use of the ${PACKAGELOCK} lock file was changed to shared to improve parallelism. However, when looking at the actual change it becomes confusing. I have included it below for reference.Quoted 24 lines
commit
ede381d56b180b384fdad98d445e5430819cfade
Author: Richard Purdie <[email protected]> Date: Wed Jan 19 11:04:15 2011 +0000package.bbclass: Take a shared lock when reading to improve
do_packageparallelismSigned-off-by Richard Purdie <[email protected]rg>
meta/classes/package.bbclass +0
-0@@ -497,7 +497,8 @@ python emit_pkgdata() { pkgdest = bb.data.getVar('PKGDEST', d, 1) pkgdatadir = bb.data.getVar('PKGDESTWORK', d, True)meta/classes/package.bbclass +0
-1- lf = bb.utils.lockfile(bb.data.expand("${PACKAGELOCK}",d))
meta/classes/package.bbclass +2
-0+ # Take shared lock since we're only reading, not writing + lf = bb.utils.lockfile(bb.data.expand("${PACKAGELOCK}",Quoted 1 line
d), True)
Here the lock is changed to shared as per the commit message.
Quoted 11 lines
data_file= pkgdatadir +bb.data.expand("/${PN}" , d)f =open(data_file, 'w')meta/classes/package.bbclass +0
-0@@ -649,6 +650,7 @@ python package_do_shlibs() { shlibs_dir = bb.data.getVar('SHLIBSDIR', d, True) shlibswork_dir = bb.data.getVar('SHLIBSWORKDIR', d, True)meta/classes/package.bbclass +1
-0+ # Take shared lock since we're only reading, not writinglf =
bb.utils.lockfile(bb.data.expand("${PACKAGELOCK}", d))Here, however, it is not changed, even though a comment is added to say that it is. Was this intentional, or just an oversight?
Quoted 10 lines
deflinux_so(root, path, file):meta/classes/package.bbclass +0
-0@@ -878,6 +880,7 @@ python package_do_pkgconfig () {if hdr == 'Requires':
pk gconfig_needed[pkg] +=exp.replace(',', ' ').split()meta/classes/package.bbclass +1
-0+ # Take shared lock since we're only reading, not writingQuoted 2 lines
lf =
bb.utils.lockfile(bb.data.expand("${PACKAGELOCK}", d))Here again a comment is added, but the code is not changed to match.
Quoted 1 line
Quoted 7 lines
Quoted 2 lines
for pkg inpackages.split():Also, what is the ${PACKAGELOCK} lock file actually protecting? With the exception of the two questionable cases above, I cannot see that the lock is taken privately anywhere else. And since it looks as the code in
package_do_shlibs()andpackage_do_pkgconfig()is not what needs protection (based on the added comments above), what is?
Sorry for not replying sooner, this was brought to my attention again.
PACKAGELOCK is there for PKGDATA_DIR which is defined as ${TMPDIR}/pkgdata/${MACHINE}, i.e. not recipe specific.
If something is reading files from pkgdata and something else writes/changes them, we used to see build failures. The lock therefore does have a purpose in guarding against this.
Looking at the code, something has gotten lost with the addition of recipe specific sysroots and the separation of do_packagedata from do_package.
I suspect it needs:
-do_packagedata[sstate-lockfile-shared] = "${PACKAGELOCK}" +do_packagedata[sstate-lockfile] = "${PACKAGELOCK}"
and the other sites you mention should be shared locks, then this would all make a lot more sense.
Cheers,
Richard