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

Pins in the manifest #743

Merged
merged 25 commits into from Jun 22, 2021
Merged

Pins in the manifest #743

merged 25 commits into from Jun 22, 2021

Conversation

mosteo
Copy link
Member

@mosteo mosteo commented May 28, 2021

Pins are now to be defined in the manifest:

[[pins]]
foo = { version = "1.3.2+bugfix" }
bar = { path = "../my/bar" }
baz = { url = "https://github.com/baz.git" } # No commit, will use current HEAD, will update on `alr update`
gru = { url = "https://gitlab.com/gru.git" commit="123456890abcdef..." } # Explicit commit, won't update

This PR is still incomplete, but is testable: the new pins work, and the old pins also work, but such old pins will be forgotten after any update (explicitly requested, or by editing the manifest), as the update process syncs with the pins in the manifest.

To do:

  • Remove old pins
  • Tests specific to the new format

Fixes #721

@Fabien-Chouteau
Copy link
Member

[[pins]]
foo = { version = "1.3.2+bugfix" }
bar = { path = "../my/bar" }
baz = { url = "https://github.com/baz.git" } # No commit, will use current HEAD, will update on alr update
gru = { url = "https://gitlab.com/gru.git" commit="123456890abcdef..." } # Explicit commit, won't update

This looks great!

src/alire/alire-solutions-diffs.adb Outdated Show resolved Hide resolved
@@ -0,0 +1,4 @@
-- Configuration for alr generated by Alire
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this folder not be added to git?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by #744

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in Alire, but they still should not be checked in here.

@onox
Copy link
Contributor

onox commented May 28, 2021

I did some testing with orka_simd, added the following to its alire.toml file:

[[pins]]
orka_types = {path = "../orka_types"}

and removing alire.lock and then running:

alr build -XORKA_SIMD_EXT="AVX2" (says solution is incomplete)
alr update (asks me to confirm new solution)

It has found the folder of orka_types 👍 (and gives me the original alire.lock again)

but if I remove alire.lock and then immediately run alr update, I get:

stderr: SYSTEM.ASSERTIONS.ASSERT_FAILURE
stderr: failed precondition from alire-solutions.ads:124
stderr: raised SYSTEM.ASSERTIONS.ASSERT_FAILURE : failed precondition from alire-solutions.ads:124

@mosteo
Copy link
Member Author

mosteo commented May 29, 2021

Thanks, @onox. The PR is still undercooked so I'll keep an eye out for these issues.

mosteo added 16 commits June 14, 2021 14:07
Minor testsuite tweak for a change in logging format
We had two confusing Update_Dependencies and Update_And_Deploy_Dependencies
that were in practice doing almost the same. There is now a single
Sync_Dependencies.
For now, these cannot work as we are going to remove the ability to edit pins
via `alr with`/`alr pin`. This functionality could be reintroduced at a later
time.
Most of those should be reimplemented in their manual edition alternative
This is purely for user comfort and will probably result in dependencies having
to be added at publish time. However, if we manage to restore command-line
pinning, we can remove that pain by adding missing dependencies at that time.
Some tests are not easily portable without support from `alr with --use`. Since
that should be easy to implement later, they are disabled for now and will be
enabled in a subsequent patch.
alire.lock Show resolved Hide resolved
doc/catalog-format-spec.md Outdated Show resolved Hide resolved
src/alire/alire-roots.adb Outdated Show resolved Hide resolved
src/alire/alire-roots.adb Outdated Show resolved Hide resolved
src/alire/alire-roots.adb Outdated Show resolved Hide resolved
@mosteo
Copy link
Member Author

mosteo commented Jun 16, 2021

This one is ready for review. There are quite some disabled tests and some portions of unreachable code. I intend to tidy that up in a follow-up patch (since this one has grown already too large), in which at least alr with --use works again, since that would allows us to keep lots of tests as-is that otherwise are hard to adapt. As this will require only to append to the manifest, as is already done for with, it should not be too complicated (fingers crossed).

Other than that, the new pins should be ready. @onox , as you have an use case, your feedback on this one would be great.

@mosteo mosteo marked this pull request as ready for review June 16, 2021 20:26
@@ -0,0 +1,4 @@
-- Configuration for alr generated by Alire
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in Alire, but they still should not be checked in here.

doc/catalog-format-spec.md Outdated Show resolved Hide resolved
doc/catalog-format-spec.md Outdated Show resolved Hide resolved
doc/catalog-format-spec.md Outdated Show resolved Hide resolved
doc/catalog-format-spec.md Show resolved Hide resolved
doc/catalog-format-spec.md Show resolved Hide resolved
src/alire/alire-dependencies-states.ads Show resolved Hide resolved
@raph-amiard
Copy link

Hello @mosteo, as I said live to Fabien, I think being able to specify a branch name in updatable pins would be a useful, or even necessary functionality in complex use cases.

Thanks for tackling this! I think it will be the tipping point for a lot of us at AdaCore to start using Alire.

@Fabien-Chouteau
Copy link
Member

@mosteo I tried to create a test crate for one of my project a found an issue. Here's how to reproduce:

$ git clone --branch new_alr_pins https://github.com/Fabien-Chouteau/bbqueue-spark
$ cd cd bbqueue-spark/tests
$ alr run main_framed
[works fine]
$ touch alire.toml # to simulate modification of the manifest
$ alr run main_framed
# Detected changes in manifest, synchronizing workspace...
Dependencies automatically updated as follows:                                                

   New solution is incomplete.
   #  atomic  ~0.3  (missing)                                    
   ## bbqueue 0.2.0 (pin=../../bbqueue-spark,upgraded from 0.1.0)
warn: Generating possibly incomplete environment because of missing dependencies
warn: Generating possibly incomplete configuration because of missing dependencies

bbqueue.gpr:3:06: unknown project file: "atomic.gpr"
bbqueue.gpr:3:06: imported by "/tmp/bbqueue-spark/bbqueue.gpr"
bbqueue.gpr:3:06: imported by "/tmp/bbqueue-spark/tests/config/tests_config.gpr"
bbqueue.gpr:3:06: imported by "/tmp/bbqueue-spark/tests/tests.gpr"
gprbuild: "tests.gpr" processing failed
error: Command ["gprbuild", "-gnatwU", "-j0", "-p", "-P", "tests.gpr"] exited with code 4
error: Build failed

@mosteo
Copy link
Member Author

mosteo commented Jun 17, 2021

Hello @mosteo, as I said live to Fabien, I think being able to specify a branch name in updatable pins would be a useful, or even necessary functionality in complex use cases.

Right, this is at my top of follow-up improvements. I'll tackle it in a follow-up PR.

@mosteo
Copy link
Member Author

mosteo commented Jun 17, 2021

@mosteo I tried to create a test crate for one of my project a found an issue. Here's how to reproduce:

I cannot reproduce it (it runs properly both times) but there's definitely something wrong because the first time around the pin is not detected. Will fix that and we'll see if it's related.

We were creating an empty lockfile, which was newer than the manifest, and thus
not triggering the expected automatic update.
This was a bug detected and corrected in the previous commit
@mosteo
Copy link
Member Author

mosteo commented Jun 17, 2021

Ready for another round, @Fabien-Chouteau . Let me know if this improved things for your test.

@onox
Copy link
Contributor

onox commented Jun 17, 2021

@mosteo Pins in wayland-ada repo seem to work, but Alire could try to simplify the relative paths of indirect dependencies, it currently just concatenates the paths. For example, in wayland_ada_info crate I get changes like this:

-path = "file:../wayland_ada_scanner"
+path = "file:../wayland_protocols_ada/../wayland_client_ada/../wayland_ada_scanner"

Not a big problem though.

@onox
Copy link
Contributor

onox commented Jun 17, 2021

I can run make just fine in wayland-ada and orka repositories, but in awt I still get some errors and exceptions.

I get a few errors when running alr build in the awt repository:

alr build -XORKA_SIMD_EXT="AVX2"
orka.gpr:1:06: unknown project file: "orka_types"
orka.gpr:1:06: imported by "/path/to/repo/orka/orka/orka.gpr"
orka.gpr:1:06: imported by "/path/to/repo/orka/orka/orka-egl.gpr"
orka.gpr:1:06: imported by "/path/to/repo/awt/awt-linux.gpr"
orka.gpr:2:06: unknown project file: "orka_simd"
orka.gpr:2:06: imported by "/path/to/repo/orka/orka/orka.gpr"
orka.gpr:2:06: imported by "/path/to/repo/orka/orka/orka-egl.gpr"
orka.gpr:2:06: imported by "/path/to/repo/awt/awt-linux.gpr"
orka.gpr:3:06: unknown project file: "orka_transforms"
orka.gpr:3:06: imported by "/path/to/repo/orka/orka/orka.gpr"
orka.gpr:3:06: imported by "/path/to/repo/orka/orka/orka-egl.gpr"
orka.gpr:3:06: imported by "/path/to/repo/awt/awt-linux.gpr"
gprbuild: "awt-linux.gpr" processing failed

If I go to ../orka/orka (orka crate in orka repo), and then run alr build -XORKA_SIMD_EXT="AVX2". It builds everything just fine.

(If I run alr update in awt repo I get a warning: warn: Missing variable in environment: os; 'other' expressions discarded.)

And I get some exceptions when running alr build -vvd in awt repo:

stderr: Pin path is not a valid directory: ../orka_egl
debug: ---8<--- Exception dump begin ---8<---
debug: ALIRE.CHECKED_ERROR
debug: /path/to/repo/awt/alire.lock:
solution
states
state  3
Loading release from manifest: ../orka/orka/alire.toml
pins
orka_egl
Pin path is not a valid directory: ../orka_egl
debug: raised ALIRE.CHECKED_ERROR : alire-stored-error:1
debug: Loading release orka_egl=0.0.0
debug: Valid root found at ../orka/orka_egl
debug: Loading release wayland_client_ada=0.0.0
stderr: Pin path is not a valid directory: ../wayland_ada_scanner
debug: ---8<--- Exception dump begin ---8<---
debug: ALIRE.CHECKED_ERROR
debug: /path/to/repo/awt/alire.lock:
solution
states
state  5
Loading release from manifest: ../wayland-ada/wayland_client_ada/alire.toml
pins
wayland_ada_scanner
Pin path is not a valid directory: ../wayland_ada_scanner
debug: raised ALIRE.CHECKED_ERROR : alire-stored-error:3
debug: Loading release wayland_cursor_ada=0.0.0
stderr: Pin path is not a valid directory: ../wayland_client_ada
debug: ---8<--- Exception dump begin ---8<---
debug: ALIRE.CHECKED_ERROR
debug: /path/to/repo/awt/alire.lock:
solution
states
state  6
Loading release from manifest: ../wayland-ada/wayland_cursor_ada/alire.toml
pins
wayland_client_ada
Pin path is not a valid directory: ../wayland_client_ada
debug: raised ALIRE.CHECKED_ERROR : alire-stored-error:5
debug: Loading release wayland_egl_ada=0.0.0
stderr: Pin path is not a valid directory: ../wayland_client_ada
debug: ---8<--- Exception dump begin ---8<---
debug: ALIRE.CHECKED_ERROR
debug: /path/to/repo/awt/alire.lock:
solution
states
state  7
Loading release from manifest: ../wayland-ada/wayland_egl_ada/alire.toml
pins
wayland_client_ada
Pin path is not a valid directory: ../wayland_client_ada
debug: raised ALIRE.CHECKED_ERROR : alire-stored-error:17

There are many more similar and duplicate exceptions, these are just a few of them. I can commit the changes to the alire.toml files if that helps you with debugging.

@onox
Copy link
Contributor

onox commented Jun 17, 2021

I have pushed some commits with pins added to the various alire.toml files.

Also get this error in awt:

orka.gpr:19:04: no value defined for "os"
orka.gpr:19:20: undefined external reference "ALR_OS"
gprbuild: "awt-linux.gpr" processing failed

even though I have the following in orka/alire.toml:

[gpr-set-externals.'case(os)']
linux = { ALR_OS = "linux" }

@mosteo
Copy link
Member Author

mosteo commented Jun 18, 2021

@mosteo Pins in wayland-ada repo seem to work, but Alire could try to simplify the relative paths of indirect dependencies, it currently just concatenates the paths. For example, in wayland_ada_info crate I get changes like this:

-path = "file:../wayland_ada_scanner"
+path = "file:../wayland_protocols_ada/../wayland_client_ada/../wayland_ada_scanner"

Actually I think this is caused by the relative path simplification functions. This is in the lockfile?

@mosteo
Copy link
Member Author

mosteo commented Jun 18, 2021

I have pushed some commits with pins added to the various alire.toml files.

Thanks, this will help me for sure.

@mosteo
Copy link
Member Author

mosteo commented Jun 18, 2021

@onox, thanks to your repos I have a clear picture of what's going wrong. I will submit the fix in a separate PR not to make this one even more complex, as it touches on recursive pin loading and the solver. I'll ping you there when ready.

@mosteo mosteo mentioned this pull request Jun 21, 2021
@mosteo mosteo changed the base branch from master to merging/pins June 22, 2021 15:54
@mosteo mosteo merged commit 13acd70 into merging/pins Jun 22, 2021
@mosteo mosteo deleted the feat/newpins branch June 22, 2021 15:55
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.

Relative dependencies get lost after updating alr
4 participants