Re: [OE-core] [PATCH] sstate.bbclass: apply proper umask when fetching from SSTATE_MIRROR

Re: [OE-core] [PATCH] sstate.bbclass: apply proper umask when fetching from SSTATE_MIRROR

[Rasmus Villemoes]

On Fri, Jun 06 2025, "Rasmus Villemoes via lists.openembedded.org" <rasmus.villemoes=prevas.dk@lists.openembedded.org> wrote:

> From: Rasmus Villemoes <ravi@prevas.dk>
>
> Currently, files and directories created under ${SSTATE_DIR} when
> fetching from an sstate mirror are not created with group write,
> unlike when the sstate artifacts are generated locally. That's
> inconsistent, and problematic when the local sstate dir is shared
> among multiple users.
>
> Wrap the fetching in a bb.utils.umask() context manager, and for simplicity
> move the mkdir of SSTATE_DIR inside that.
>

Ping.

[PATCH] sstate.bbclass: apply proper umask when fetching from SSTATE_MIRROR

[Rasmus Villemoes]

From: Rasmus Villemoes <ravi@prevas.dk>

Currently, files and directories created under ${SSTATE_DIR} when
fetching from an sstate mirror are not created with group write,
unlike when the sstate artifacts are generated locally. That's
inconsistent, and problematic when the local sstate dir is shared
among multiple users.

Wrap the fetching in a bb.utils.umask() context manager, and for simplicity
move the mkdir of SSTATE_DIR inside that.

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 meta/classes-global/sstate.bbclass | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/meta/classes-global/sstate.bbclass b/meta/classes-global/sstate.bbclass
index 1d7b033b80..2968cc4c2e 100644
--- a/meta/classes-global/sstate.bbclass
+++ b/meta/classes-global/sstate.bbclass
@@ -726,7 +726,6 @@ def pstaging_fetch(sstatefetch, d):
     localdata = bb.data.createCopy(d)
 
     dldir = localdata.expand("${SSTATE_DIR}")
-    bb.utils.mkdirhier(dldir)
 
     localdata.delVar('MIRRORS')
     localdata.setVar('FILESPATH', dldir)
@@ -746,16 +745,19 @@ def pstaging_fetch(sstatefetch, d):
     if bb.utils.to_boolean(d.getVar("SSTATE_VERIFY_SIG"), False):
         uris += ['file://{0}.sig;downloadfilename={0}.sig'.format(sstatefetch)]
 
-    for srcuri in uris:
-        localdata.delVar('SRC_URI')
-        localdata.setVar('SRC_URI', srcuri)
-        try:
-            fetcher = bb.fetch2.Fetch([srcuri], localdata, cache=False)
-            fetcher.checkstatus()
-            fetcher.download()
+    with bb.utils.umask(0o002):
+        bb.utils.mkdirhier(dldir)
 
-        except bb.fetch2.BBFetchException:
-            pass
+        for srcuri in uris:
+            localdata.delVar('SRC_URI')
+            localdata.setVar('SRC_URI', srcuri)
+            try:
+                fetcher = bb.fetch2.Fetch([srcuri], localdata, cache=False)
+                fetcher.checkstatus()
+                fetcher.download()
+
+            except bb.fetch2.BBFetchException:
+                pass
 
 def sstate_setscene(d):
     shared_state = sstate_state_fromvars(d)
-- 
2.49.0

Re: [PATCH] sstate.bbclass: apply proper umask when fetching from SSTATE_MIRROR

[Richard Purdie]

On Fri, 2025-06-06 at 11:39 +0200, Rasmus Villemoes wrote:
> From: Rasmus Villemoes <ravi@prevas.dk>
> 
> Currently, files and directories created under ${SSTATE_DIR} when
> fetching from an sstate mirror are not created with group write,
> unlike when the sstate artifacts are generated locally. That's
> inconsistent, and problematic when the local sstate dir is shared
> among multiple users.
> 
> Wrap the fetching in a bb.utils.umask() context manager, and for simplicity
> move the mkdir of SSTATE_DIR inside that.
> 
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
> ---
>  meta/classes-global/sstate.bbclass | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/meta/classes-global/sstate.bbclass b/meta/classes-global/sstate.bbclass
> index 1d7b033b80..2968cc4c2e 100644
> --- a/meta/classes-global/sstate.bbclass
> +++ b/meta/classes-global/sstate.bbclass
> @@ -726,7 +726,6 @@ def pstaging_fetch(sstatefetch, d):
>      localdata = bb.data.createCopy(d)
>  
>      dldir = localdata.expand("${SSTATE_DIR}")
> -    bb.utils.mkdirhier(dldir)
>  
>      localdata.delVar('MIRRORS')
>      localdata.setVar('FILESPATH', dldir)
> @@ -746,16 +745,19 @@ def pstaging_fetch(sstatefetch, d):
>      if bb.utils.to_boolean(d.getVar("SSTATE_VERIFY_SIG"), False):
>          uris += ['file://{0}.sig;downloadfilename={0}.sig'.format(sstatefetch)]
>  
> -    for srcuri in uris:
> -        localdata.delVar('SRC_URI')
> -        localdata.setVar('SRC_URI', srcuri)
> -        try:
> -            fetcher = bb.fetch2.Fetch([srcuri], localdata, cache=False)
> -            fetcher.checkstatus()
> -            fetcher.download()
> +    with bb.utils.umask(0o002):
> +        bb.utils.mkdirhier(dldir)
>  
> -        except bb.fetch2.BBFetchException:
> -            pass
> +        for srcuri in uris:
> +            localdata.delVar('SRC_URI')
> +            localdata.setVar('SRC_URI', srcuri)
> +            try:
> +                fetcher = bb.fetch2.Fetch([srcuri], localdata, cache=False)
> +                fetcher.checkstatus()
> +                fetcher.download()
> +
> +            except bb.fetch2.BBFetchException:
> +                pass
>  

Probably the right thing to do, I just can't help wonder if 0o002
should be in a variable like SSTATE_UMASK or something just so it isn't
so hardcoded. I did note we already have other references to it.

Cheers,

Richard

Re: [OE-core] [PATCH] sstate.bbclass: apply proper umask when fetching from SSTATE_MIRROR

[Ryan Eatmon]

On 6/13/2025 11:30 AM, Richard Purdie via lists.openembedded.org wrote:
> On Fri, 2025-06-06 at 11:39 +0200, Rasmus Villemoes wrote:
>> From: Rasmus Villemoes <ravi@prevas.dk>
>>
>> Currently, files and directories created under ${SSTATE_DIR} when
>> fetching from an sstate mirror are not created with group write,
>> unlike when the sstate artifacts are generated locally. That's
>> inconsistent, and problematic when the local sstate dir is shared
>> among multiple users.
>>
>> Wrap the fetching in a bb.utils.umask() context manager, and for simplicity
>> move the mkdir of SSTATE_DIR inside that.
>>
>> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
>> ---
>>   meta/classes-global/sstate.bbclass | 22 ++++++++++++----------
>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/meta/classes-global/sstate.bbclass b/meta/classes-global/sstate.bbclass
>> index 1d7b033b80..2968cc4c2e 100644
>> --- a/meta/classes-global/sstate.bbclass
>> +++ b/meta/classes-global/sstate.bbclass
>> @@ -726,7 +726,6 @@ def pstaging_fetch(sstatefetch, d):
>>       localdata = bb.data.createCopy(d)
>>   
>>       dldir = localdata.expand("${SSTATE_DIR}")
>> -    bb.utils.mkdirhier(dldir)
>>   
>>       localdata.delVar('MIRRORS')
>>       localdata.setVar('FILESPATH', dldir)
>> @@ -746,16 +745,19 @@ def pstaging_fetch(sstatefetch, d):
>>       if bb.utils.to_boolean(d.getVar("SSTATE_VERIFY_SIG"), False):
>>           uris += ['file://{0}.sig;downloadfilename={0}.sig'.format(sstatefetch)]
>>   
>> -    for srcuri in uris:
>> -        localdata.delVar('SRC_URI')
>> -        localdata.setVar('SRC_URI', srcuri)
>> -        try:
>> -            fetcher = bb.fetch2.Fetch([srcuri], localdata, cache=False)
>> -            fetcher.checkstatus()
>> -            fetcher.download()
>> +    with bb.utils.umask(0o002):
>> +        bb.utils.mkdirhier(dldir)
>>   
>> -        except bb.fetch2.BBFetchException:
>> -            pass
>> +        for srcuri in uris:
>> +            localdata.delVar('SRC_URI')
>> +            localdata.setVar('SRC_URI', srcuri)
>> +            try:
>> +                fetcher = bb.fetch2.Fetch([srcuri], localdata, cache=False)
>> +                fetcher.checkstatus()
>> +                fetcher.download()
>> +
>> +            except bb.fetch2.BBFetchException:
>> +                pass
>>   
> 
> Probably the right thing to do, I just can't help wonder if 0o002
> should be in a variable like SSTATE_UMASK or something just so it isn't
> so hardcoded. I did note we already have other references to it.

+1 for never hard coding things.  Make it a variable, just in case.


> Cheers,
> 
> Richard
> 
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#218618): https://lists.openembedded.org/g/openembedded-core/message/218618
> Mute This Topic: https://lists.openembedded.org/mt/113500418/6551054
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [reatmon@ti.com]
> -=-=-=-=-=-=-=-=-=-=-=-
> 

-- 
Ryan Eatmon                reatmon@ti.com
-----------------------------------------
Texas Instruments, Inc.  -  LCPD  -  MGTS

Re: [PATCH] sstate.bbclass: apply proper umask when fetching from SSTATE_MIRROR

[Richard Purdie]

On Fri, 2025-06-06 at 11:39 +0200, Rasmus Villemoes wrote:
> From: Rasmus Villemoes <ravi@prevas.dk>
> 
> Currently, files and directories created under ${SSTATE_DIR} when
> fetching from an sstate mirror are not created with group write,
> unlike when the sstate artifacts are generated locally. That's
> inconsistent, and problematic when the local sstate dir is shared
> among multiple users.
> 
> Wrap the fetching in a bb.utils.umask() context manager, and for simplicity
> move the mkdir of SSTATE_DIR inside that.
> 
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
> ---
>  meta/classes-global/sstate.bbclass | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/meta/classes-global/sstate.bbclass b/meta/classes-global/sstate.bbclass
> index 1d7b033b80..2968cc4c2e 100644
> --- a/meta/classes-global/sstate.bbclass
> +++ b/meta/classes-global/sstate.bbclass
> @@ -726,7 +726,6 @@ def pstaging_fetch(sstatefetch, d):
>      localdata = bb.data.createCopy(d)
>  
>      dldir = localdata.expand("${SSTATE_DIR}")
> -    bb.utils.mkdirhier(dldir)
>  
>      localdata.delVar('MIRRORS')
>      localdata.setVar('FILESPATH', dldir)
> @@ -746,16 +745,19 @@ def pstaging_fetch(sstatefetch, d):
>      if bb.utils.to_boolean(d.getVar("SSTATE_VERIFY_SIG"), False):
>          uris += ['file://{0}.sig;downloadfilename={0}.sig'.format(sstatefetch)]
>  
> -    for srcuri in uris:
> -        localdata.delVar('SRC_URI')
> -        localdata.setVar('SRC_URI', srcuri)
> -        try:
> -            fetcher = bb.fetch2.Fetch([srcuri], localdata, cache=False)
> -            fetcher.checkstatus()
> -            fetcher.download()
> +    with bb.utils.umask(0o002):
> +        bb.utils.mkdirhier(dldir)
>  
> -        except bb.fetch2.BBFetchException:
> -            pass
> +        for srcuri in uris:
> +            localdata.delVar('SRC_URI')
> +            localdata.setVar('SRC_URI', srcuri)
> +            try:
> +                fetcher = bb.fetch2.Fetch([srcuri], localdata, cache=False)
> +                fetcher.checkstatus()
> +                fetcher.download()
> +
> +            except bb.fetch2.BBFetchException:
> +                pass
>  

I appreciate this sounds crazy but this is causing some kind of
regression being reported in our automated testing. Specifically,
running:

oe-selftest -r sstatetests.SStateCreation -j 1

fails for me locally with this applied, as it does here in CI:

https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/1767

Worryingly, if I run:

oeselftest -r sstatetests.SStateCreation.test_sstate_creation_distro_specific_pass -j 1

i.e. a specific failing test, that fails even without the patch
applied, and it shouldn't so there is something odd going on here even
before the patch.

We're going to have to get to the bottom of this before I can merge the
patch.

Cheers,

Richard

Re: [OE-core] [PATCH] sstate.bbclass: apply proper umask when fetching from SSTATE_MIRROR

[Rasmus Villemoes]

On Sun, Jun 15 2025, "Richard Purdie via lists.openembedded.org" <richard.purdie=linuxfoundation.org@lists.openembedded.org> wrote:

> On Fri, 2025-06-06 at 11:39 +0200, Rasmus Villemoes wrote:
>> From: Rasmus Villemoes <ravi@prevas.dk>
>> 
>> Currently, files and directories created under ${SSTATE_DIR} when
>> fetching from an sstate mirror are not created with group write,
>> unlike when the sstate artifacts are generated locally. That's
>> inconsistent, and problematic when the local sstate dir is shared
>> among multiple users.
>> 
>> Wrap the fetching in a bb.utils.umask() context manager, and for simplicity
>> move the mkdir of SSTATE_DIR inside that.
>> 

> I appreciate this sounds crazy but this is causing some kind of
> regression being reported in our automated testing. Specifically,
> running:
>
> oe-selftest -r sstatetests.SStateCreation -j 1
>
> fails for me locally with this applied, as it does here in CI:
>
> https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/1767
>
> Worryingly, if I run:
>
> oeselftest -r sstatetests.SStateCreation.test_sstate_creation_distro_specific_pass -j 1
>
> i.e. a specific failing test, that fails even without the patch
> applied, and it shouldn't so there is something odd going on here even
> before the patch.
>
> We're going to have to get to the bottom of this before I can merge the
> patch.

Fair enough.

I'm not sure I can help much, but looking at that test code, something
feels a little fishy:

        # Now we'll walk the tree to check the mode and see if things are incorrect.
        badperms = []
        for root, dirs, files in os.walk(self.sstate_path):
            for directory in dirs:
                if (os.stat(os.path.join(root, directory)).st_mode & 0o777) != 0o775:
                    badperms.append(os.path.join(root, directory))

        # Return to original umask
        os.umask(orig_umask)

        if should_pass:
            self.assertTrue(badperms , msg="Found sstate directories with the wrong permissions: %s (found %s)" % (', '.join(map(str, targets)), str(badperms)))

So badperms is a list of stuff we've found that has ended up with wrong
permissions. But AFAICT, we assert that badperms is 'truish',
i.e. non-empty.

Just guessing: Perhaps the reason that passes on CI is exactly because
it always ends up getting the artifacts from an SSTATE_MIRROR (thus with
"wrong" permissions in the current state of the sstate mirror fetching),
while perhaps you don't when you run it locally, and thus you do see the
failure because your local dirs end up being created with the right 0775
perms, and badperms ends up empty?

While in there, the current_umask() helper seems redundant; just doing

  orig_umask = os.umask(0o022)

would do exactly what one wants.

Rasmus

Re: [OE-core] [PATCH] sstate.bbclass: apply proper umask when fetching from SSTATE_MIRROR

[Richard Purdie]

On Tue, 2025-06-17 at 09:41 +0200, Rasmus Villemoes wrote:
> On Sun, Jun 15 2025, "Richard Purdie via lists.openembedded.org" <richard.purdie=linuxfoundation.org@lists.openembedded.org> wrote:
> 
> > On Fri, 2025-06-06 at 11:39 +0200, Rasmus Villemoes wrote:
> > > From: Rasmus Villemoes <ravi@prevas.dk>
> > > 
> > > Currently, files and directories created under ${SSTATE_DIR} when
> > > fetching from an sstate mirror are not created with group write,
> > > unlike when the sstate artifacts are generated locally. That's
> > > inconsistent, and problematic when the local sstate dir is shared
> > > among multiple users.
> > > 
> > > Wrap the fetching in a bb.utils.umask() context manager, and for simplicity
> > > move the mkdir of SSTATE_DIR inside that.
> > > 
> 
> > I appreciate this sounds crazy but this is causing some kind of
> > regression being reported in our automated testing. Specifically,
> > running:
> > 
> > oe-selftest -r sstatetests.SStateCreation -j 1
> > 
> > fails for me locally with this applied, as it does here in CI:
> > 
> > https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/1767
> > 
> > Worryingly, if I run:
> > 
> > oeselftest -r sstatetests.SStateCreation.test_sstate_creation_distro_specific_pass -j 1
> > 
> > i.e. a specific failing test, that fails even without the patch
> > applied, and it shouldn't so there is something odd going on here even
> > before the patch.
> > 
> > We're going to have to get to the bottom of this before I can merge the
> > patch.
> 
> Fair enough.
> 
> I'm not sure I can help much, but looking at that test code, something
> feels a little fishy:
> 
>         # Now we'll walk the tree to check the mode and see if things are incorrect.
>         badperms = []
>         for root, dirs, files in os.walk(self.sstate_path):
>             for directory in dirs:
>                 if (os.stat(os.path.join(root, directory)).st_mode & 0o777) != 0o775:
>                     badperms.append(os.path.join(root, directory))
> 
>         # Return to original umask
>         os.umask(orig_umask)
> 
>         if should_pass:
>             self.assertTrue(badperms , msg="Found sstate directories with the wrong permissions: %s (found %s)" % (', '.join(map(str, targets)), str(badperms)))
> 
> So badperms is a list of stuff we've found that has ended up with wrong
> permissions. But AFAICT, we assert that badperms is 'truish',
> i.e. non-empty.
> 
> Just guessing: Perhaps the reason that passes on CI is exactly because
> it always ends up getting the artifacts from an SSTATE_MIRROR (thus with
> "wrong" permissions in the current state of the sstate mirror fetching),
> while perhaps you don't when you run it locally, and thus you do see the
> failure because your local dirs end up being created with the right 0775
> perms, and badperms ends up empty?
> 
> While in there, the current_umask() helper seems redundant; just doing
> 
>   orig_umask = os.umask(0o022)
> 
> would do exactly what one wants.

I finally got to the bottom of the issues. NATIVELSBSTRING handling was
causing weirdness when running the tests which was than masking other
problems. Once I fixed that, I could unravel the problem with badperms,
which was just broken as you mentioned. In the end I cleaned up the
umask handling there using bb.utils.umask and combined some of the test
cases for clarity too. I've sent a couple of patches.

This should mean your patch can now merge as it is correct IMO and the
tests were just broken/breaking.

Cheers,

Richard

Re: [OE-core] [PATCH] sstate.bbclass: apply proper umask when fetching from SSTATE_MIRROR

[Rasmus Villemoes]

On Wed, Jun 25 2025, Richard Purdie <richard.purdie@linuxfoundation.org> wrote:

> On Tue, 2025-06-17 at 09:41 +0200, Rasmus Villemoes wrote:
>> On Sun, Jun 15 2025, "Richard Purdie via lists.openembedded.org" <richard.purdie=linuxfoundation.org@lists.openembedded.org> wrote:
>> 
>> > On Fri, 2025-06-06 at 11:39 +0200, Rasmus Villemoes wrote:
>> > > From: Rasmus Villemoes <ravi@prevas.dk>
>> > > 
>> > > Currently, files and directories created under ${SSTATE_DIR} when
>> > > fetching from an sstate mirror are not created with group write,
>> > > unlike when the sstate artifacts are generated locally. That's
>> > > inconsistent, and problematic when the local sstate dir is shared
>> > > among multiple users.
>> > > 
>> > > Wrap the fetching in a bb.utils.umask() context manager, and for simplicity
>> > > move the mkdir of SSTATE_DIR inside that.
>> > > 
>> 
>> > I appreciate this sounds crazy but this is causing some kind of
>> > regression being reported in our automated testing. Specifically,
>> > running:
>> > 
>> > oe-selftest -r sstatetests.SStateCreation -j 1
>> > 
>> > fails for me locally with this applied, as it does here in CI:
>> > 
>> > https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/1767
>> > 
>> > Worryingly, if I run:
>> > 
>> > oeselftest -r sstatetests.SStateCreation.test_sstate_creation_distro_specific_pass -j 1
>> > 
>> > i.e. a specific failing test, that fails even without the patch
>> > applied, and it shouldn't so there is something odd going on here even
>> > before the patch.
>> > 
>> > We're going to have to get to the bottom of this before I can merge the
>> > patch.
>> 
>
> I finally got to the bottom of the issues. NATIVELSBSTRING handling was
> causing weirdness when running the tests which was than masking other
> problems. Once I fixed that, I could unravel the problem with badperms,
> which was just broken as you mentioned. In the end I cleaned up the
> umask handling there using bb.utils.umask and combined some of the test
> cases for clarity too. I've sent a couple of patches.
>
> This should mean your patch can now merge as it is correct IMO and the
> tests were just broken/breaking.

Thanks for taking the time to work through this. I was hoping the needed
fixups were small enough (or localized enough) that my patch, along with
those fixups, could be eligible for walnascar. What's your take on that?

I got into this because we currently have to set BB_DEFAULT_UMASK =
"002" to get the sstate dir perms right on our shared build
infrastructure, but it turns out that that cure is worse than the
disease as I wrote here:
https://lore.kernel.org/openembedded-core/87wm9r1wcx.fsf@prevas.dk/

Thanks,
Rasmus

Re: [OE-core] [PATCH] sstate.bbclass: apply proper umask when fetching from SSTATE_MIRROR

[Richard Purdie]

On Thu, 2025-06-26 at 08:51 +0200, Rasmus Villemoes wrote:
> On Wed, Jun 25 2025, Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> 
> > On Tue, 2025-06-17 at 09:41 +0200, Rasmus Villemoes wrote:
> > > On Sun, Jun 15 2025, "Richard Purdie via lists.openembedded.org" <richard.purdie=linuxfoundation.org@lists.openembedded.org> wrote:
> > > 
> > > > On Fri, 2025-06-06 at 11:39 +0200, Rasmus Villemoes wrote:
> > > > > From: Rasmus Villemoes <ravi@prevas.dk>
> > > > > 
> > > > > Currently, files and directories created under ${SSTATE_DIR} when
> > > > > fetching from an sstate mirror are not created with group write,
> > > > > unlike when the sstate artifacts are generated locally. That's
> > > > > inconsistent, and problematic when the local sstate dir is shared
> > > > > among multiple users.
> > > > > 
> > > > > Wrap the fetching in a bb.utils.umask() context manager, and for simplicity
> > > > > move the mkdir of SSTATE_DIR inside that.
> > > > > 
> > > 
> > > > I appreciate this sounds crazy but this is causing some kind of
> > > > regression being reported in our automated testing. Specifically,
> > > > running:
> > > > 
> > > > oe-selftest -r sstatetests.SStateCreation -j 1
> > > > 
> > > > fails for me locally with this applied, as it does here in CI:
> > > > 
> > > > https://autobuilder.yoctoproject.org/valkyrie/#/builders/35/builds/1767
> > > > 
> > > > Worryingly, if I run:
> > > > 
> > > > oeselftest -r sstatetests.SStateCreation.test_sstate_creation_distro_specific_pass -j 1
> > > > 
> > > > i.e. a specific failing test, that fails even without the patch
> > > > applied, and it shouldn't so there is something odd going on here even
> > > > before the patch.
> > > > 
> > > > We're going to have to get to the bottom of this before I can merge the
> > > > patch.
> > > 
> > 
> > I finally got to the bottom of the issues. NATIVELSBSTRING handling was
> > causing weirdness when running the tests which was than masking other
> > problems. Once I fixed that, I could unravel the problem with badperms,
> > which was just broken as you mentioned. In the end I cleaned up the
> > umask handling there using bb.utils.umask and combined some of the test
> > cases for clarity too. I've sent a couple of patches.
> > 
> > This should mean your patch can now merge as it is correct IMO and the
> > tests were just broken/breaking.
> 
> Thanks for taking the time to work through this. I was hoping the needed
> fixups were small enough (or localized enough) that my patch, along with
> those fixups, could be eligible for walnascar. What's your take on that?

A quick poll of a few developers on the review call as we discussed
these changes suggested it would be a candidate to backport.

> I got into this because we currently have to set BB_DEFAULT_UMASK =
> "002" to get the sstate dir perms right on our shared build
> infrastructure, but it turns out that that cure is worse than the
> disease as I wrote here:
> https://lore.kernel.org/openembedded-core/87wm9r1wcx.fsf@prevas.dk/

I have been meaning to look at that a bit. We haven't really supported
changing the overall default umask as the potential for unintended
changes is significant. It could probably be made to work but someone
would need to put a lot of development work in.

Cheers,

Richard

Re: [OE-core] [PATCH] sstate.bbclass: apply proper umask when fetching from SSTATE_MIRROR

[Rasmus Villemoes]

On Thu, Jun 26 2025, Richard Purdie <richard.purdie@linuxfoundation.org> wrote:

> On Thu, 2025-06-26 at 08:51 +0200, Rasmus Villemoes wrote:
>> On Wed, Jun 25 2025, Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
>> 
>> > This should mean your patch can now merge as it is correct IMO and the
>> > tests were just broken/breaking.
>> 
>> Thanks for taking the time to work through this. I was hoping the needed
>> fixups were small enough (or localized enough) that my patch, along with
>> those fixups, could be eligible for walnascar. What's your take on that?
>
> A quick poll of a few developers on the review call as we discussed
> these changes suggested it would be a candidate to backport.

Great.

>> I got into this because we currently have to set BB_DEFAULT_UMASK =
>> "002" to get the sstate dir perms right on our shared build
>> infrastructure, but it turns out that that cure is worse than the
>> disease as I wrote here:
>> https://lore.kernel.org/openembedded-core/87wm9r1wcx.fsf@prevas.dk/
>
> I have been meaning to look at that a bit. We haven't really supported
> changing the overall default umask as the potential for unintended
> changes is significant.

Yes, I realized :) But since the sstate dirs/files were not (always)
created with the permissions we wanted, I thought that BB_DEFAULT_UMASK
was the proper knob. Only when that turned out to have unwanted side
effects did I dig deeper and found that the sstate code already tries to
create files/dirs with group write, but with the mirror case overlooked
(which explained the odd mix of permissions we observed).

We also share our DL_DIR, and the fetch code doesn't seem to ensure 
group write is allowed - perhaps we just need to set do_fetch[umask],
but is it intended that DL_DIR should be sharable? The problem occurs
when one user would build a newer git-based recipe and thus would need
write access to DL_DIR/git2/foo-bar.git/ and that dir had originally
been created by some other user.

> It could probably be made to work but someone would need to put a lot
> of development work in.

Well, we'll be happy to just get rid of our non-default
BB_DEFAULT_UMASK.

But it might be an idea to ensure that do_rootfs[umask] and
do_image[umask] are 022, regardless of the global default umask, or at
least have some warning in place if they aren't. I haven't really tested
setting those; it's possible that the BB_DEFAULT_UMASK setting ends up
polluting stuff before it gets into the rootfs.

Rasmus

Re: [OE-core] [PATCH] sstate.bbclass: apply proper umask when fetching from SSTATE_MIRROR

[Richard Purdie]

On Thu, 2025-06-26 at 14:50 +0200, Rasmus Villemoes wrote:
> On Thu, Jun 26 2025, Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> 
> > On Thu, 2025-06-26 at 08:51 +0200, Rasmus Villemoes wrote:
> > > On Wed, Jun 25 2025, Richard Purdie <richard.purdie@linuxfoundation.org> wrote:
> > > 
> > > > This should mean your patch can now merge as it is correct IMO and the
> > > > tests were just broken/breaking.
> > > 
> > > Thanks for taking the time to work through this. I was hoping the needed
> > > fixups were small enough (or localized enough) that my patch, along with
> > > those fixups, could be eligible for walnascar. What's your take on that?
> > 
> > A quick poll of a few developers on the review call as we discussed
> > these changes suggested it would be a candidate to backport.
> 
> Great.
> 
> > > I got into this because we currently have to set BB_DEFAULT_UMASK =
> > > "002" to get the sstate dir perms right on our shared build
> > > infrastructure, but it turns out that that cure is worse than the
> > > disease as I wrote here:
> > > https://lore.kernel.org/openembedded-core/87wm9r1wcx.fsf@prevas.dk/
> > 
> > I have been meaning to look at that a bit. We haven't really supported
> > changing the overall default umask as the potential for unintended
> > changes is significant.
> 
> Yes, I realized :) But since the sstate dirs/files were not (always)
> created with the permissions we wanted, I thought that BB_DEFAULT_UMASK
> was the proper knob. Only when that turned out to have unwanted side
> effects did I dig deeper and found that the sstate code already tries to
> create files/dirs with group write, but with the mirror case overlooked
> (which explained the odd mix of permissions we observed).
> 
> We also share our DL_DIR, and the fetch code doesn't seem to ensure 
> group write is allowed - perhaps we just need to set do_fetch[umask],
> but is it intended that DL_DIR should be sharable?

It is intended to be shared the same way as SSTATE_DIR is. Setting the
umask for that could be the right call.

>  The problem occurs
> when one user would build a newer git-based recipe and thus would need
> write access to DL_DIR/git2/foo-bar.git/ and that dir had originally
> been created by some other user.

Right, that is supposed to work.

> > It could probably be made to work but someone would need to put a lot
> > of development work in.
> 
> Well, we'll be happy to just get rid of our non-default
> BB_DEFAULT_UMASK.
> 
> But it might be an idea to ensure that do_rootfs[umask] and
> do_image[umask] are 022, regardless of the global default umask, or at
> least have some warning in place if they aren't. I haven't really tested
> setting those; it's possible that the BB_DEFAULT_UMASK setting ends up
> polluting stuff before it gets into the rootfs.

The default umask definitely has effects in other areas such as
packaging. I think I'd be relcutant to start setting it on a per task
basis unless there is a specific reason such as do_fetch.

I still think we should put the sstate umask into a variable too.

Cheers,

Richard