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

androidenv: enhance script for the generated expressions #82067

Merged
merged 4 commits into from Mar 16, 2020

Conversation

lucafavatella
Copy link
Contributor

@lucafavatella lucafavatella commented Mar 8, 2020

Motivation for this change

Make the generation of androidenv generated expressions more reproducible.

This started by extracting the non-controversial bits from PR #58131 , that almost got approval in Apr 2019 but was not merged for the presence of bits requiring further refinement.

(I was considering to propose an unrelated androidenv PR and I read in the PR template re helping reviewing PRs, so I am triaging androidenv-related PRs and trying to bring them forward.)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
    • Not applicable.
  • Built on platform(s) - Not applicable in itself, tested via androidenv: update generated expressions #82118 so please refer to that.
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests) - Not applicable
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip" - Not applicable
  • Tested execution of all binary files (usually in ./result/bin/) - Not applicable
  • Determined the impact on package closure size (by running nix path-info -S before and after) - Not applicable
  • Ensured that relevant documentation is up to date
    • doc/languages-frameworks/android.section.md#updating-the-generated-expressions
  • Fits CONTRIBUTING.md.

lucafavatella and others added 3 commits March 9, 2020 03:43
Update generate.sh to run using nix-shell. Also make it fail with
meaningful output instead of writing empty output files.

This is extracted from https://github.com/NixOS/nixpkgs PR 58131.

This relies on the shebang being used.
Updated with fixes for `convertsystemimages.xsl`:
- Use `type-details/codename` if it exists, falling back to
  `type-details/api-level`: this results in "Q" rather than "28" for
  preview images
- Use `<xsl:text>` elements to control whitespace in the output.

This is extracted from https://github.com/NixOS/nixpkgs PR 58131.
Entry `<remotePackage path="cmdline-tools;latest">` resulted in a
duplicated `"cmdline-tools"."1.0"`.
@Mic92 Mic92 requested a review from svanderburg March 9, 2020 18:43
@lucafavatella lucafavatella marked this pull request as ready for review March 9, 2020 19:18
@lucafavatella
Copy link
Contributor Author

@tadfisher @svanderburg @numinit These are generate.sh-related changes extracted from PR #58131 so - strictly speaking - not meant to have functional changes (that I segregated in PR #82118 - still draft while I check doc) : could you please (re-)review these (#82067) changes so we move things forward re androidenv PRs?

@lucafavatella
Copy link
Contributor Author

As evident in PR #82118, in order to smoke test successfully the expressions generated by the script amended in this PR (that are not in this PR, but in #82118 ) I had to manually delete a few of the generated entries. I suggest not to wait for the process to be fully implemented as automated before we merge PRs otherwise PRs may stay hanging (then requiring new rebase and review ...).

@lucafavatella lucafavatella mentioned this pull request Mar 9, 2020
10 tasks
@svanderburg
Copy link
Member

These can go in. The smoke tests succeed and changes make sense.

@lucafavatella I will make some updates to my tests. I noticed that the documentation is slightly outdated here and there.

@svanderburg
Copy link
Member

svanderburg commented Mar 16, 2020

About the generation: it seems that we are facing all kinds of new problems compared to several months ago. I did some investigation and the sad conclusion is that you need some kind of imperative program to properly process it.

Some packages, for example, have the same name and version identifiers, but different zip files. For example some entries are split between operaing systems. The XSL stylesheets make the assumption that there is only one entry per package/version. That used to work in 99% of all cases, but now it seems that the amount of packages that deviate from this has grown.

It seems that Google also has weird ways to configure package repository metadata.

I think manual tweaking for now is unavoidable, until we have adjusted the XSL stylesheets to become more robust. Or when we have come up with a more robust conversion strategy,

So I agree, let's just integrate this. As long as the test pass I'm fine with it.

@svanderburg
Copy link
Member

I'll also add to my personal TODO list to investigate the conversion process so that we actually can get the conversion done without tweaking, but I need to find the time for that :)

@svanderburg svanderburg merged commit 52c89d0 into NixOS:master Mar 16, 2020
@lucafavatella lucafavatella deleted the androidenv-generate branch March 19, 2020 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants