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/21 20:05:51 UTC
[GitHub] [buildstream-plugins] nanonyme opened a new pull request, #29: Revert "Enable setting build-dir"
nanonyme opened a new pull request, #29:
URL: https://github.com/apache/buildstream-plugins/pull/29
This reverts commit 6c17a7a0f3c1f49c725413d34b0d5cfe83b46ae5.
It is not possible for freedesktop-sdk to use autotools.yml
without breaking API.
--
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 #29: Revert "Enable setting build-dir"
Posted by GitBox <gi...@apache.org>.
nanonyme commented on PR #29:
URL: https://github.com/apache/buildstream-plugins/pull/29#issuecomment-1223651923
Yeah. Maybe it should be documented with conf-root it is expected to be used together with command-subdir
--
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] staehle commented on pull request #29: Revert "Enable setting build-dir"
Posted by GitBox <gi...@apache.org>.
staehle commented on PR #29:
URL: https://github.com/apache/buildstream-plugins/pull/29#issuecomment-1276640907
Oop, ran into this when updating plugins in my project for the new 1.95.3 release. This change broke API for me :)
We were relying on the 1.95.1 behavior, which while broken for OOT builds, was easy to work around. Previously had this in a .yml file included by project.conf:
```yml
# Autotools plugin is broken when trying to do out-of-tree builds. Attempt fix:
elements:
autotools:
variables:
configure: "%{build-root}/configure %{conf-args}"
```
Out-of-tree Autotools builds are mandatory for cross-toolchain components: Binutils, GCC, GLibC
For anyone else this may affect, you can update to the new plugins release version and use this updated shim in your project instead to revert to the old autotools behavior:
```yml
elements:
autotools:
variables:
build-dir: "."
configure: "%{build-root}/configure %{conf-args}"
make-args: >-
-C "%{build-dir}"
config:
configure-commands:
- |
%{autogen}
- |
mkdir -p "%{build-dir}"
cd "%{build-dir}"
%{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] nanonyme commented on pull request #29: Revert "Enable setting build-dir"
Posted by GitBox <gi...@apache.org>.
nanonyme commented on PR #29:
URL: https://github.com/apache/buildstream-plugins/pull/29#issuecomment-1221616095
So I'm now officially giving up on the thought that it's possible to add build-dir support upstream. Ping @jjardon. I don't think we should leave variables behind that don't work so creating this revert. Freedesktop-sdk will need to override probably at least configure, autogen and conf-cmd, make-args to fix this.
--
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 #29: Revert "Enable setting build-dir"
Posted by GitBox <gi...@apache.org>.
gtristan commented on PR #29:
URL: https://github.com/apache/buildstream-plugins/pull/29#issuecomment-1223601289
I'm not sure if we should revert this.
It would be nice if the built-in `command-subdir` feature worked in perfect harmony with this `build-dir` feature of `autotools` plugin, but I'm not convinced that these two working perfectly together is a blocker to adding this `build-dir` feature.
It seems to me:
* Fairly frequent that people like to use `srcdir != builddir`
* Less frequent that we need to build something in a subdirectory
* An odd example of building only the JS implementation from a mozilla checkout comes to mind
* Quite improbable that we need to use `srcdir != builddir` along with `command-subdir`
* And even then, these default build instructions are just *helpful* for covering the majority of cases, it isn't horrible that a user needs to manually override some stuff in corner cases
--
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 #29: Revert "Enable setting build-dir"
Posted by GitBox <gi...@apache.org>.
gtristan commented on PR #29:
URL: https://github.com/apache/buildstream-plugins/pull/29#issuecomment-1223642572
Ok I see what happened... this dates back to #512
It appears that the correct way to do `srcdir != builddir` is to use `command-subdir` to do it.
An example/test was added in 0d04e1b7db7e7612a2d2ca13652e7c425e519650 and the documentation was updated to sort of explain how to do `srcdir != builddir` in [BuildElement documentation](https://docs.buildstream.build/master/buildstream.buildelement.html#location-for-configuring-the-project)
It looks indeed that the prescribed method for doing `srcdir != builddir` would be:
```yaml
variables:
command-subdir: "_build"
conf-root: "%{build-root}"
```
And that we should probably indeed just revert this patch, which tries to reimplement `builddir != srcdir` another way.
--
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 #29: Revert "Enable setting build-dir"
Posted by GitBox <gi...@apache.org>.
gtristan commented on PR #29:
URL: https://github.com/apache/buildstream-plugins/pull/29#issuecomment-1223610666
Looking at the current code, it appears that this would work quite nicely so long as we just documented `build-dir` to mention that `conf-root` should **also** be set to `..`.
Right now, since [`conf-root` is `.`](https://github.com/apache/buildstream/blob/master/src/buildstream/data/projectconfig.yaml#L39), it appears the plugin would break if we tried to use `build-dir`, because the code would expand to:
```sh
mkdir -p _build
cd _build
./configure .... arguments ...
```
So I would conclude that, the currently committed code is broken for `build-dir` usage, but *can* be useful if the user *also* sets `conf-root` to `..`.
Since we don't use absolute paths in any case, it would appear that this strategy would *equally* work if the user has tried to use `command-subdir`.
Have I misread the situation ?
--
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 #29: Revert "Enable setting build-dir"
Posted by GitBox <gi...@apache.org>.
gtristan commented on PR #29:
URL: https://github.com/apache/buildstream-plugins/pull/29#issuecomment-1223719128
> Note in this case you would set conf-root as `..`, most likely. Autogen still needs some fixing. IMHO it should enter conf-root as the first thing instead using it for paths since autogen scripts are typically not designed to be run from another directory. (but configure is)
Well, it is tested with the `amhello.tgz` tarball but this might have an existing configure script.
Probably worth adding a test to ensure that like `autoreconf -ivf %{conf-root}` works properly, but I would presume it should.
--
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] staehle commented on pull request #29: Revert "Enable setting build-dir"
Posted by GitBox <gi...@apache.org>.
staehle commented on PR #29:
URL: https://github.com/apache/buildstream-plugins/pull/29#issuecomment-1276753118
Oh apologies, I should have wrote an example element as well for how I'm using "build-dir". As a project override, it would essentially just replace the autotools' `%{conf-root}` with `%{build-root}` in the `configure` variable as an absolute path instead of a relative one. Maybe that causes issues in other autotoolsy projects, I don't know, but it works fine for my implementations of Binutils, GCC, GLibC at least.
In my binutils.bst, for example, just defining 'build-dir' like this would then work to automatically build it OOT:
```yml
variables:
build-dir: build_dir
```
and would produce this command list in the log:
```
[--:--:--] START toolchain/pieces/binutils.bst: Running commands
if [ -x ./configure ]; then true;
elif [ -x ./autogen ]; then ./autogen;
elif [ -x ./autogen.sh ]; then ./autogen.sh;
elif [ -x ./bootstrap ]; then ./bootstrap;
elif [ -x ./bootstrap.sh ]; then ./bootstrap.sh;
else autoreconf -ivf .;
fi
mkdir -p "build_dir"
cd "build_dir"
/buildstream/robs/toolchain/pieces/binutils.bst/configure --build=<snip-configure-args>
make -C build_dir
make -C build_dir -j1 DESTDIR="/buildstream-install" install
if false || false; then
find "/buildstream-install" -name "*.la" -print0 | while read -d '' -r file; do
if grep '^shouldnotlink=yes$' "${file}" &>/dev/null; then
if false; then
echo "Removing ${file}."
rm "${file}"
else
echo "Not removing ${file}."
fi
else
if false; then
echo "Removing ${file}."
rm "${file}"
else
echo "Not removing ${file}."
fi
fi
done
fi
```
But anyway, it's fine, it's not that big of a deal. Just letting you know that OOT autotools builds might be a needed feature, otherwise some projects are going to come up with hacks like the one I made :)
--
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 #29: Revert "Enable setting build-dir"
Posted by GitBox <gi...@apache.org>.
nanonyme commented on PR #29:
URL: https://github.com/apache/buildstream-plugins/pull/29#issuecomment-1223683341
Note in this case you would set conf-root as `..`, most likely. Autogen still needs some fixing. IMHO it should enter conf-root as the first thing instead using it for paths since autogen scripts are typically not designed to be run from another directory.
--
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 #29: Revert "Enable setting build-dir"
Posted by GitBox <gi...@apache.org>.
gtristan merged PR #29:
URL: https://github.com/apache/buildstream-plugins/pull/29
--
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 #29: Revert "Enable setting build-dir"
Posted by GitBox <gi...@apache.org>.
gtristan commented on PR #29:
URL: https://github.com/apache/buildstream-plugins/pull/29#issuecomment-1223728186
Related to this, I've also pushed some clarifications and fixes to the build element docs: https://github.com/apache/buildstream/pull/1734
--
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 #29: Revert "Enable setting build-dir"
Posted by GitBox <gi...@apache.org>.
nanonyme commented on PR #29:
URL: https://github.com/apache/buildstream-plugins/pull/29#issuecomment-1276722277
Well, I am reasonably confident making a solution that actually works every API requires rewriting the autotools API from scratch. There are just too many interconnected variables to have a clean build-dir implementation. It sounds to me though that you are actually disabling build-dir with above snippet in any case?
--
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 #29: Revert "Enable setting build-dir"
Posted by GitBox <gi...@apache.org>.
nanonyme commented on PR #29:
URL: https://github.com/apache/buildstream-plugins/pull/29#issuecomment-1223625565
@gtristan The problem is that conf-root has been implemented in bst2 so that it also affects autogen path, not just configure path. I would need to break API somehow to split these. If you fix configure through conf-root, you break autogen
--
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 #29: Revert "Enable setting build-dir"
Posted by GitBox <gi...@apache.org>.
nanonyme commented on PR #29:
URL: https://github.com/apache/buildstream-plugins/pull/29#issuecomment-1223631781
I might to be honest be able to also fix this through rewriting the autogen block. But not sure at this point
--
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