diff mbox

filter BR2_DEFCONFIG out of defconfig files

Message ID 1391523148-30887-1-git-send-email-jeremy.rosen@openwide.fr
State Superseded
Headers show

Commit Message

Jeremy Rosen Feb. 4, 2014, 2:12 p.m. UTC
When it is set, BR2_DEFCONFIG is saved like any config variable.
However, at this point it is a completely expanded, absolute path
It is not a good idea to save absolute paths in defconfig files
moreover it makes little sense to save the defconfig location
within the defconfig

Signed-off-by: Jérémy Rosen <jeremy.rosen@openwide.fr>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

Comments

Thomas De Schampheleire Feb. 5, 2014, 11:54 a.m. UTC | #1
Hi,

On Tue, Feb 4, 2014 at 3:12 PM, Jérémy Rosen <jeremy.rosen@openwide.fr> wrote:
> When it is set, BR2_DEFCONFIG is saved like any config variable.
> However, at this point it is a completely expanded, absolute path
> It is not a good idea to save absolute paths in defconfig files
> moreover it makes little sense to save the defconfig location
> within the defconfig

The entire BR2_DEFCONFIG mechanism has never done what I expected.
What I want is that 'make savedefconfig' (or a similar command) saves
the configuration back to where it originated from. BR2_DEFCONFIG does
more or less that, but only if you set the configuration initially
with 'make defconfig BR2_DEFCONFIG=...' instead of the normal 'make
xxxx_defconfig'. Isn't this possible to achieve?

Best regards,
Thomas
Yann E. MORIN Feb. 5, 2014, 6:22 p.m. UTC | #2
Jérémy, All,

On 2014-02-04 15:12 +0100, Jérémy Rosen spake thusly:
> When it is set, BR2_DEFCONFIG is saved like any config variable.
> However, at this point it is a completely expanded, absolute path
> It is not a good idea to save absolute paths in defconfig files
> moreover it makes little sense to save the defconfig location
> within the defconfig
> 
> Signed-off-by: Jérémy Rosen <jeremy.rosen@openwide.fr>
> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Makefile b/Makefile
> index 4a675d4..f63a058 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -772,6 +772,7 @@ savedefconfig: $(BUILD_DIR)/buildroot-config/conf outputmakefile
>  	@$(COMMON_CONFIG_ENV) $< \
>  		--savedefconfig=$(if $(DEFCONFIG),$(DEFCONFIG),$(CONFIG_DIR)/defconfig) \
>  		$(CONFIG_CONFIG_IN)
> +	sed -i '/^BR2_DEFCONFIG=/d' $(DEFCONFIG)

Should be:
    $(SED) '/blabla/'

$(SED) already contains -i

Regards,
Yann E. MORIN.
Arnout Vandecappelle Feb. 5, 2014, 10:43 p.m. UTC | #3
On 04/02/14 15:12, Jérémy Rosen wrote:
> When it is set, BR2_DEFCONFIG is saved like any config variable.
> However, at this point it is a completely expanded, absolute path
> It is not a good idea to save absolute paths in defconfig files
> moreover it makes little sense to save the defconfig location
> within the defconfig
> 
> Signed-off-by: Jérémy Rosen <jeremy.rosen@openwide.fr>

 Jeremy, I guess this patch is superseded by mine?

 Regards,
 Arnout

> ---
>  Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Makefile b/Makefile
> index 4a675d4..f63a058 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -772,6 +772,7 @@ savedefconfig: $(BUILD_DIR)/buildroot-config/conf outputmakefile
>  	@$(COMMON_CONFIG_ENV) $< \
>  		--savedefconfig=$(if $(DEFCONFIG),$(DEFCONFIG),$(CONFIG_DIR)/defconfig) \
>  		$(CONFIG_CONFIG_IN)
> +	sed -i '/^BR2_DEFCONFIG=/d' $(DEFCONFIG)
>  
>  # check if download URLs are outdated
>  source-check:
>
Arnout Vandecappelle Feb. 5, 2014, 10:46 p.m. UTC | #4
On 05/02/14 12:54, Thomas De Schampheleire wrote:
> Hi,
> 
> On Tue, Feb 4, 2014 at 3:12 PM, Jérémy Rosen <jeremy.rosen@openwide.fr> wrote:
>> When it is set, BR2_DEFCONFIG is saved like any config variable.
>> However, at this point it is a completely expanded, absolute path
>> It is not a good idea to save absolute paths in defconfig files
>> moreover it makes little sense to save the defconfig location
>> within the defconfig
> 
> The entire BR2_DEFCONFIG mechanism has never done what I expected.
> What I want is that 'make savedefconfig' (or a similar command) saves
> the configuration back to where it originated from. BR2_DEFCONFIG does
> more or less that, but only if you set the configuration initially
> with 'make defconfig BR2_DEFCONFIG=...' instead of the normal 'make
> xxxx_defconfig'. Isn't this possible to achieve?

 Sure, quite easily. However, is that really what we want?


make rpi_defconfig
add some packages
test test test
add more packages
test test test
make savedefconfig

Oops I've just overwritten rpi_defconfig.


 It's probably what you want when using BR2_EXTERNAL, but not when using
a buildroot defconfig. Unless, of course, you don't use BR2_EXTERNAL but
add directly to the buildroot directory...


 Regards,
 Arnout
Yann E. MORIN Feb. 5, 2014, 11:18 p.m. UTC | #5
Arnout, All,

On 2014-02-05 23:46 +0100, Arnout Vandecappelle spake thusly:
> make rpi_defconfig
> add some packages
> test test test
> add more packages
> test test test
> make savedefconfig
> 
> Oops I've just overwritten rpi_defconfig.

Oh no! Not rpi_defconfig!

Sorry, couldn't resist... ;-)

Regards,
Yann E. MORIN.
Thomas De Schampheleire Feb. 6, 2014, 7:50 a.m. UTC | #6
Hi Arnout,

On Wed, Feb 5, 2014 at 11:46 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 05/02/14 12:54, Thomas De Schampheleire wrote:
>> Hi,
>>
>> On Tue, Feb 4, 2014 at 3:12 PM, Jérémy Rosen <jeremy.rosen@openwide.fr> wrote:
>>> When it is set, BR2_DEFCONFIG is saved like any config variable.
>>> However, at this point it is a completely expanded, absolute path
>>> It is not a good idea to save absolute paths in defconfig files
>>> moreover it makes little sense to save the defconfig location
>>> within the defconfig
>>
>> The entire BR2_DEFCONFIG mechanism has never done what I expected.
>> What I want is that 'make savedefconfig' (or a similar command) saves
>> the configuration back to where it originated from. BR2_DEFCONFIG does
>> more or less that, but only if you set the configuration initially
>> with 'make defconfig BR2_DEFCONFIG=...' instead of the normal 'make
>> xxxx_defconfig'. Isn't this possible to achieve?
>
>  Sure, quite easily. However, is that really what we want?
>
>
> make rpi_defconfig
> add some packages
> test test test
> add more packages
> test test test
> make savedefconfig
>
> Oops I've just overwritten rpi_defconfig.
>
>
>  It's probably what you want when using BR2_EXTERNAL, but not when using
> a buildroot defconfig. Unless, of course, you don't use BR2_EXTERNAL but
> add directly to the buildroot directory...

The question is how you define 'a buildroot defconfig'. Not so long
ago, our recommendation for 'real' projects was to also put project
defconfigs in configs/. For these defconfigs, I do expect the
described behavior. It's true that an end-user with a raspberrypi
should create his own private defconfig, from which point the
described behavior is again desirable. A buildroot developer improving
the raspberrypi defconfig (on purpose) wants the behavior as well.
So the only situation where it is not desirable is when you start from
a non-project defconfig and make private modifications.

One way forward is to introduce a new config variable, enabled on all
non-project defconfigs, stating that we do not want this behavior. The
buildroot developer improving the raspberrypi defconfig will have to
do something special, but this may be acceptable.

The project developer now has a useful improvement in workflow.

What do you think of that?

Thanks,
Thomas
Jeremy Rosen Feb. 6, 2014, 8:25 a.m. UTC | #7
----- Mail original -----
> On 04/02/14 15:12, Jérémy Rosen wrote:
> > When it is set, BR2_DEFCONFIG is saved like any config variable.
> > However, at this point it is a completely expanded, absolute path
> > It is not a good idea to save absolute paths in defconfig files
> > moreover it makes little sense to save the defconfig location
> > within the defconfig
> > 
> > Signed-off-by: Jérémy Rosen <jeremy.rosen@openwide.fr>
> 
>  Jeremy, I guess this patch is superseded by mine?
> 
yes, I should have pointed that out, sorry
Arnout Vandecappelle Feb. 6, 2014, 9:15 p.m. UTC | #8
On 06/02/14 08:50, Thomas De Schampheleire wrote:
> Hi Arnout,
> 
> On Wed, Feb 5, 2014 at 11:46 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
>> On 05/02/14 12:54, Thomas De Schampheleire wrote:
>>> Hi,
>>>
>>> On Tue, Feb 4, 2014 at 3:12 PM, Jérémy Rosen <jeremy.rosen@openwide.fr> wrote:
>>>> When it is set, BR2_DEFCONFIG is saved like any config variable.
>>>> However, at this point it is a completely expanded, absolute path
>>>> It is not a good idea to save absolute paths in defconfig files
>>>> moreover it makes little sense to save the defconfig location
>>>> within the defconfig
>>>
>>> The entire BR2_DEFCONFIG mechanism has never done what I expected.
>>> What I want is that 'make savedefconfig' (or a similar command) saves
>>> the configuration back to where it originated from. BR2_DEFCONFIG does
>>> more or less that, but only if you set the configuration initially
>>> with 'make defconfig BR2_DEFCONFIG=...' instead of the normal 'make
>>> xxxx_defconfig'. Isn't this possible to achieve?
>>
>>  Sure, quite easily. However, is that really what we want?
>>
>>
>> make rpi_defconfig
>> add some packages
>> test test test
>> add more packages
>> test test test
>> make savedefconfig
>>
>> Oops I've just overwritten rpi_defconfig.
>>
>>
>>  It's probably what you want when using BR2_EXTERNAL, but not when using
>> a buildroot defconfig. Unless, of course, you don't use BR2_EXTERNAL but
>> add directly to the buildroot directory...
> 
> The question is how you define 'a buildroot defconfig'. Not so long
> ago, our recommendation for 'real' projects was to also put project
> defconfigs in configs/. For these defconfigs, I do expect the
> described behavior. It's true that an end-user with a raspberrypi
> should create his own private defconfig, from which point the
> described behavior is again desirable. A buildroot developer improving
> the raspberrypi defconfig (on purpose) wants the behavior as well.
> So the only situation where it is not desirable is when you start from
> a non-project defconfig and make private modifications.
> 
> One way forward is to introduce a new config variable, enabled on all
> non-project defconfigs, stating that we do not want this behavior. The
> buildroot developer improving the raspberrypi defconfig will have to
> do something special, but this may be acceptable.
> 
> The project developer now has a useful improvement in workflow.
> 
> What do you think of that?

 OK, so in most cases we really want the original defconfig to be
overwritten on savedefconfig. It is only when it's one of buildroot's own
config files that we prefer to protect the user.

 I'm not happy with the solution you propose, though, because it adds
complexity and yet another config option for not much of a feature. How
about this one, though: for users who clone the git tree, it's not much
of a problem since they'll see they made a mistake; for the release
tarballs, we can just make configs/*_defconfig readonly while creating
the tarball.


 Regards,
 Arnout
diff mbox

Patch

diff --git a/Makefile b/Makefile
index 4a675d4..f63a058 100644
--- a/Makefile
+++ b/Makefile
@@ -772,6 +772,7 @@  savedefconfig: $(BUILD_DIR)/buildroot-config/conf outputmakefile
 	@$(COMMON_CONFIG_ENV) $< \
 		--savedefconfig=$(if $(DEFCONFIG),$(DEFCONFIG),$(CONFIG_DIR)/defconfig) \
 		$(CONFIG_CONFIG_IN)
+	sed -i '/^BR2_DEFCONFIG=/d' $(DEFCONFIG)
 
 # check if download URLs are outdated
 source-check: