You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@buildstream.apache.org by GitBox <gi...@apache.org> on 2022/08/11 16:40:15 UTC
[GitHub] [buildstream-plugins] nanonyme opened a new pull request, #25: Enable setting build-dir
nanonyme opened a new pull request, #25:
URL: https://github.com/apache/buildstream-plugins/pull/25
build-dir is disabled by default and can be enabled with project conf
or through element
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] gtristan commented on pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
gtristan commented on PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#issuecomment-1212236094
I think this would be more flexible and convenient if:
* the default for `build-dir` was `.`
* the `make` and `make-install` variables were extended to contain `-C %{build-dir}`
it may also be partly a matter of taste, I think this would be much more elegant than having the script jumping around into different directories.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] gtristan commented on pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
gtristan commented on PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#issuecomment-1214069307
> @gtristan this should now be ready. Independent of this all cache keys are changed with bst master.
I've updated `buildstream-plugins` and it's cache key test.
Can you please rebase this branch ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] nanonyme commented on pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
nanonyme commented on PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#issuecomment-1214186645
@gtristan rebase is now done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] nanonyme commented on a diff in pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
nanonyme commented on code in PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#discussion_r944695624
##########
src/buildstream_plugins/elements/autotools.yaml:
##########
@@ -93,6 +94,8 @@ config:
- |
%{autogen}
- |
+ mkdir -p "%{build-dir}"
+ cd "%{build-dir}"
%{configure}
Review Comment:
@abderrahim did I misunderstand what you were after?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] abderrahim commented on a diff in pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
abderrahim commented on code in PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#discussion_r944619965
##########
src/buildstream_plugins/elements/autotools.yaml:
##########
@@ -93,6 +94,8 @@ config:
- |
%{autogen}
- |
+ mkdir -p "%{build-dir}"
+ cd "%{build-dir}"
%{configure}
Review Comment:
Don't we need a `cd -` here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] gtristan commented on a diff in pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
gtristan commented on code in PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#discussion_r943769204
##########
src/buildstream_plugins/elements/autotools.py:
##########
@@ -51,6 +51,14 @@
conf-global: |
--disable-gtk-doc --disable-static
+By default build-dir is not used. To improve probability build does not
+tamper with sources, you can set build-dir like
+
+.. code:: yaml
+
+ variables:
+ build-dir: bst_build-dir
Review Comment:
Nit: this is a strange suggestion with mixed `-` and `_`
perhaps `_build` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] nanonyme commented on a diff in pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
nanonyme commented on code in PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#discussion_r944622226
##########
src/buildstream_plugins/elements/autotools.yaml:
##########
@@ -93,6 +94,8 @@ config:
- |
%{autogen}
- |
+ mkdir -p "%{build-dir}"
+ cd "%{build-dir}"
%{configure}
Review Comment:
Turns out we don't. These aren't actually concatenated into one shell script.
##########
src/buildstream_plugins/elements/autotools.yaml:
##########
@@ -93,6 +94,8 @@ config:
- |
%{autogen}
- |
+ mkdir -p "%{build-dir}"
+ cd "%{build-dir}"
%{configure}
Review Comment:
These aren't actually concatenated into one shell script.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] gtristan commented on a diff in pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
gtristan commented on code in PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#discussion_r943698550
##########
src/buildstream_plugins/elements/autotools.yaml:
##########
@@ -93,20 +94,30 @@ config:
- |
%{autogen}
- |
- %{configure}
+ if [ -n "%{build-dir}" ]; then
+ mkdir -p "%{build-dir}"
+ cd "%{build-dir}"
+ fi
+ %{configure})
Review Comment:
Trailing `)` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] nanonyme commented on a diff in pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
nanonyme commented on code in PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#discussion_r944622226
##########
src/buildstream_plugins/elements/autotools.yaml:
##########
@@ -93,6 +94,8 @@ config:
- |
%{autogen}
- |
+ mkdir -p "%{build-dir}"
+ cd "%{build-dir}"
%{configure}
Review Comment:
These aren't actually concatenated into one shell script but run in subshells. It's unnecessary to restore path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] nanonyme commented on pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
nanonyme commented on PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#issuecomment-1212301696
Jumping still required for configure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] gtristan commented on a diff in pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
gtristan commented on code in PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#discussion_r943698964
##########
src/buildstream_plugins/elements/autotools.yaml:
##########
@@ -93,20 +94,30 @@ config:
- |
%{autogen}
- |
- %{configure}
+ if [ -n "%{build-dir}" ]; then
+ mkdir -p "%{build-dir}"
+ cd "%{build-dir}"
+ fi
+ %{configure})
# Commands for building the software
#
build-commands:
- |
+ if [ -n "%{build-dir}" ]; then
+ cd "%{build-dir}"
+ fi
%{make}
# Commands for installing the software into a
# destination folder
#
install-commands:
- |
- %{make-install}
+ if [ -n "%{build-dir}" ]; then
+ cd "%{build-dir}"
+ fi
+ %{make-install})
Review Comment:
Trailing `)` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] gtristan commented on a diff in pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
gtristan commented on code in PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#discussion_r944702960
##########
src/buildstream_plugins/elements/autotools.yaml:
##########
@@ -93,6 +94,8 @@ config:
- |
%{autogen}
- |
+ mkdir -p "%{build-dir}"
+ cd "%{build-dir}"
%{configure}
Review Comment:
Indeed we do not need `cd -`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] nanonyme commented on pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
nanonyme commented on PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#issuecomment-1212226184
Continuation of https://github.com/apache/buildstream-plugins/pull/21
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] nanonyme commented on a diff in pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
nanonyme commented on code in PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#discussion_r943771329
##########
src/buildstream_plugins/elements/autotools.py:
##########
@@ -51,6 +51,14 @@
conf-global: |
--disable-gtk-doc --disable-static
+By default build-dir is not used. To improve probability build does not
+tamper with sources, you can set build-dir like
+
+.. code:: yaml
+
+ variables:
+ build-dir: bst_build-dir
Review Comment:
I guess to be consistent with cmake and meson we could suggest _builddir
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] gtristan commented on pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
gtristan commented on PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#issuecomment-1212307527
Overall looks good to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] gtristan merged pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
gtristan merged PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] nanonyme commented on a diff in pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
nanonyme commented on code in PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#discussion_r943769690
##########
src/buildstream_plugins/elements/autotools.py:
##########
@@ -51,6 +51,14 @@
conf-global: |
--disable-gtk-doc --disable-static
+By default build-dir is not used. To improve probability build does not
+tamper with sources, you can set build-dir like
+
+.. code:: yaml
+
+ variables:
+ build-dir: bst_build-dir
Review Comment:
Ah, yes. True.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] nanonyme commented on pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
nanonyme commented on PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#issuecomment-1213042797
@gtristan this should now be ready. Independent of this all cache keys are changed with bst master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [buildstream-plugins] nanonyme commented on a diff in pull request #25: Enable setting build-dir
Posted by GitBox <gi...@apache.org>.
nanonyme commented on code in PR #25:
URL: https://github.com/apache/buildstream-plugins/pull/25#discussion_r944622226
##########
src/buildstream_plugins/elements/autotools.yaml:
##########
@@ -93,6 +94,8 @@ config:
- |
%{autogen}
- |
+ mkdir -p "%{build-dir}"
+ cd "%{build-dir}"
%{configure}
Review Comment:
These aren't actually concatenated into one shell script but run in subshells. (as was educated to me by @gtristan) It's unnecessary to restore path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org