Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

meson: allow building from shallow clones #122

Closed
wants to merge 1 commit into from
Closed

meson: allow building from shallow clones #122

wants to merge 1 commit into from

Conversation

petermarko
Copy link
Contributor

Shallow clones are useful for air-gapped build environments and to ensure various compliance tasks.

When building from shallow clone, tag is not available and version defaults to git hash.
Problem is that some builds check DTC version and fail the comparison. Example is https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git Which fails to build with following error:
dtc version too old (039a994), you need at least version 1.4.4

Drop --always from git describe command, see
https://github.com/mesonbuild/meson/blob/1.3.0/mesonbuild/utils/universal.py#L773 This will make it more closer to build via Makefile.

When building from shallow clone, tag is not available
and version defaults to git hash.
Problem is that some builds check DTC version and fail the comparison.
Example is https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git
Which fails to build with following error:
dtc version too old (039a994), you need at least version 1.4.4

Drop --always from git describe command, see
https://github.com/mesonbuild/meson/blob/1.3.0/mesonbuild/utils/universal.py#L773
This will make it more closer to build via Makefile.

Signed-off-by: Peter Marko <[email protected]>
@dgibson
Copy link
Owner

dgibson commented Dec 18, 2023

The change looks good to me, but I'm running into an unrelated failure in the pylibfdt tests :(. Looks like something has changed in Fedora that's subtly breaking something.

@petermarko
Copy link
Contributor Author

Looks like python 3.12 changed swig return values (adding also function return code, not only return parameters).
Now the question is if coming swig 4.2.0 will fix this (by adding compatibility with python 3.12) or we need to adapt bindings code to handle python version and drop the first value or just check the python version in testcase and expect or 2 or 3 return values.
Unfortunately I have no knowledge of python bindings, so I don't know what should be the correct way to fix this.

@dgibson
Copy link
Owner

dgibson commented Dec 27, 2023

Yeah.. unfortunately I don't know a lot about the Python bindings either. @sjg20 care to weigh in?

@sjg20
Copy link
Contributor

sjg20 commented Dec 27, 2023

Yeah.. unfortunately I don't know a lot about the Python bindings either. @sjg20 care to weigh in?

Gosh, it is surprising that swig could change such a thing without some sort of backwards-compatible feature. Is there an indication that swig 4.2.0 will fix this? My reading of the change log for that swig did not produce certainty that the problem will be fixed there ("Python 3.12 support added." - is that it?)

I am not sure if the swig change is a bug, or a feature. If the latter, then checking the Python version would work, I suppose. But it is quite unfortunate. So far as I can tell, this changes the API of pylibfdt, so get_mem_rsv() returns a different result. This will presumably break most code that uses pylibfdt, unless I am missing something? In fact I wonder how pylibfdt works on Fedora today?

@dgibson
Copy link
Owner

dgibson commented Jan 25, 2024

Yeah.. unfortunately I don't know a lot about the Python bindings either. @sjg20 care to weigh in?

Gosh, it is surprising that swig could change such a thing without some sort of backwards-compatible feature. Is there an indication that swig 4.2.0 will fix this? My reading of the change log for that swig did not produce certainty that the problem will be fixed there ("Python 3.12 support added." - is that it?)

:(

I am not sure if the swig change is a bug, or a feature. If the latter, then checking the Python version would work, I suppose. But it is quite unfortunate. So far as I can tell, this changes the API of pylibfdt, so get_mem_rsv() returns a different result. This will presumably break most code that uses pylibfdt, unless I am missing something? In fact I wonder how pylibfdt works on Fedora today?

Looks like it doesn't: https://github.com/dgibson/dtc/actions/runs/7649045871/job/20842803203

@zumbi
Copy link

zumbi commented Jan 30, 2024

Yeah.. unfortunately I don't know a lot about the Python bindings either. @sjg20 care to weigh in?

Gosh, it is surprising that swig could change such a thing without some sort of backwards-compatible feature. Is there an indication that swig 4.2.0 will fix this? My reading of the change log for that swig did not produce certainty that the problem will be fixed there ("Python 3.12 support added." - is that it?)

I am not sure if the swig change is a bug, or a feature. If the latter, then checking the Python version would work, I suppose. But it is quite unfortunate. So far as I can tell, this changes the API of pylibfdt, so get_mem_rsv() returns a different result. This will presumably break most code that uses pylibfdt, unless I am missing something? In fact I wonder how pylibfdt works on Fedora today?

I tried swig 4.1.0 and 4.2.0, the testcase for pylibfdt fails with both versions using Python3.12, but works fine on previous Python version 3.11.

@petermarko
Copy link
Contributor Author

Well, python upgrades just break things. There was a hope that "compatibility with python 3.12" will fix it, but it didn't.
I don't think I'm able to fix this issue properly, so we either merge this red or wait for someone with skills to fix it...

@blmaier
Copy link
Contributor

blmaier commented Mar 18, 2024

The pylibfdt failure on Fedora should be fixed by #128

@dgibson
Copy link
Owner

dgibson commented Mar 19, 2024

Despite the pylibfdt failure, I merged this some time ago. Closing.

@dgibson dgibson closed this Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants