#feat(engine): support expanding variable...

1 messages · Page 1 of 1 (latest)

placid radish
#

Maybe you can start by regenerating the PHP sdk btw:

dagger -m .dagger call sdk php generate -o . in your PR, just to see if that fixes the problem

safe moon
#

I'll have a look

#

If I'm understanding correctly, the issue is the engine being outdated for PhpSdk and PhpSdkDev.

Short term fix is probably to simply update the versions of the engine it relies on.
Long term fix is sorting out the PhpSdkDev to do the "spin up" itself

#

I will confirm this first once it finishes the generate command you mentioned.

If it does I will put in a small PR to simply update the engine versions if that's the issue.
I had been updating the engine versions in my other PRs but they are waiting on reviews before they can be merged:
https://github.com/dagger/dagger/pulls/charjr

GitHub
Pull requests · dagger/dagger

An engine to run your pipelines in containers. Contribute to dagger/dagger development by creating an account on GitHub.

placid radish
#

I'll review them

late ruin
placid radish
#

@safe moon I reviewed 2 of the Pr you did with some suggestions 😉 I let the 3rd one to Helder since he already reviewed multiple times, he got full context on that

safe moon
#

Silly question, how do I perform codegen again?

#

I haven't used the command in so long. Or was that what the dagger call sdk php generate -o command should be doing?

placid radish
safe moon
#

the -m .dagger part doesn't seem to work for me

placid radish
#

Are you doing it at the root of the dagger repo?

#

Did you also rebase the PR?

safe moon
#

I was trying to work off of rajat's pr

late ruin
#

I think we now need to use -m . (instead of -m .dagger)

placid radish
#

Hmm .dagger shoudl work

#

Ohhh

#

dagger.json has move to the root again

#

Then yeah, . should be better

safe moon
#

Ah thank you @late ruin @wraith sparrow It's running now

safe moon
placid radish
#

That's strange though cause I can do:

dagger -m .dagger functions
✔ connect 20.1s
✔ initialize 1m35.1s

Name                  Description
check                 Check that everything works. Use this as CI entrypoint.
cli                   Develop the Dagger CLI
dev                   Creates a dev container that has a running CLI connected to a dagger engine
dev-export            Creates an static dev build
docs                  Develop the Dagger documentation
engine                Develop the Dagger engine container
git-dir               -
go                    Dagger's Go toolchain
helm                  Develop the Dagger helm chart
mod-codegen           When set, module codegen is automatically applied when retrieving the Dagger source code
mod-codegen-targets   -
ref                   -
scripts               Run Dagger scripts
sdk                   Develop Dagger SDKs
source                Return the Dagger source code
tag                   -
test                  Run all tests
version               -
with-mod-codegen      Enable module auto-codegen when retrieving the dagger source code
safe moon
#

Ah I can as well. I'm honestly not sure then.. maybe I entered it wrong but I thought I copy pasted it XD

late ruin
#

maybe the error is because the source dir context changes between running from root vs .dagger dir?

sterile stratus
#

we do this for python today

#

@late ruin are you able to add something similar to the php dev module?

#

you can't do it with a constructor yet, since constructors are supported, but you could add a withBase, or even just add it as an arg to Test

safe moon
sterile stratus
#

yes, but even with the constructors merged, they're not releases

safe moon
#

Ah.

sterile stratus
#

and we need this to work on v0.13.3, which is part of the problem 😛

late ruin
#

are you able to add something similar to the php dev module?

let me timebox this for a couple of hours and try out. Will reach out if I run into issues 🙂

sterile stratus
#

thanks thanks ❤️

#

i'd use the python module for inspiration, it should just be passing throught the "base" container similar to how it's done there

dreamy wing
#

If it's a new PR targeting 13.4, why does it need to work on 13.3?

late ruin
late ruin
#

the fix is to use dev engine for running CI tests

sterile stratus
#

this works yes, but can we not take the approach of python like i suggested above? we should still call the phps dev module for testing if possible

sterile stratus
#

the php dev module is imported into our ci

#

therefore it has to work on both of these versions

late ruin
#

sure, we can do that. i may have to add more functions to php sdk, that should be ok right?

sterile stratus
#

yup, it's just to the dev module

late ruin
#

right.

#

thats what i meant. sorry.

sterile stratus
#

no worries!

late ruin
safe moon
#

@late ruin Hi that is a mistake! I will fix that!

the constructor should not have a return type

#

Should look like this:

    #[DaggerFunction]
    public function __construct(
        // whatever arguments you have here
    ) {
        // ...
    }
safe moon
late ruin
#

I've been trying to debug a failure for php sdk changes (to use dev engine), and found that using option ExperimentalPrivilegedNesting: true somehow results in using old engine. but with it to be false, it works as expected

#

@sterile stratus @safe moon - any pointers why that might be the case and is it ok to turn off the flag in SDK for tests?

safe moon
late ruin
#

yeah, some help to move the PR would be great: https://github.com/dagger/dagger/pull/8427. It seems now PHP tests are passing, but some test is failing in PHP-dev. (and some flakes in Rust and other CI jobs 😦 )

safe moon
#

I think @sterile stratus's advice is going to be the right approach, I'm just trying to follow how it works.

late ruin
#

yeah, now the changes are following Justin's advice. I have the container with the custom installer, and it seems it is the right container. (btw, PHP, PHP-dev and Rust are green now in CI after retrying runs , just one last run left. 🤞 )

sterile stratus
#

It looks mostly good, but we can't use a constructor here - that constructor is going to be ignored for the php job right?

#

Since our ci is using v0.13.3, which doesn't have constructor support (in php)

#

since it seems to be passing though, i'm very confused as to why this works

safe moon
#

Since the PHP one is actually just an alias if I recall

sterile stratus
#

oh

#

i see, right php is pulling directly off of main and isn't pinned to anything 🤔

#

i suppose that's fine, but we should probably pin it to a specific commit, instead of relying on always using main

safe moon
safe moon
sterile stratus
#

specifically, just php will default to the v0.13.3 tag

#

we should update it to be php@<current main commit>.
when you add a new php feature, you can use it in the dev module by bumping the commit in the version there

#

i don't think there are plans to introduce breaking changes to the php sdk right now, but if we ever do it, then the pins here will be useful

late ruin
#

will this change impact the current PR?

#

i would really love to have this PR merged once the CI is green. pretty please.

#

🙂

safe moon
sterile stratus
sterile stratus
late ruin
#

the CI is green on the PR now. if nothing else is blocking, could we pls merge that PR?

sterile stratus
#

i have some comments, would you rather do them in this pr, or as a follow-up

late ruin
#

oh, I am happy to address them here.

#

will push a fix 👍

sterile stratus
#

otherwise, lgtm

late ruin
#

Hi Justin, I think i have addressed those comments and also added some additional testcases,

#

thank you for catching them.

late ruin
#

Hi @safe moon with the constructor changes in PHP Dev, I am getting following error:


Stdout:
[ERROR] DaggerFunction "__construct" cannot be supported without a return type ```

did something changed recently regarding this? it was working for me earlier.
safe moon
# late ruin Hi <@1229425915675807797> with the constructor changes in PHP Dev, I am getting ...

It should not have changed.

Looking at that error, it looks like it's not recognising it as the constructor. This is exactly what would happen before constructors were supported.

What version of dagger is it trying to use? To me it looks as though the PhpSdkDev is actually working off of 0.13.3? cc @sterile stratus

You could try testing it by changing the sdk in sdk/php/dev/dagger.json to "sdk": "php@05cab0d6502599671a9da7d338011edd3a76129c",. This is the commit hash when constructor support got merged.

late ruin
#

ok. let me try that. but what is surprising is that this was working yesterday. so maybe something else changed somewhere else. let me verify with the commit you suggested

#

yeah, that seems to have fixed it.

#

🙏 thank you

#

let me cross check what might have triggered this changed behavior

#

let me double check something else as well.

#

yeah, that seems like it. 😦

sterile stratus
safe moon
# late ruin if SDK == "github.com/dagger/dagger/sdk/php" -> it uses main if SDK == "php" -> ...

Ah! That makes sense now.

Yes I hadn't updated to use the alias yet on dev. I apologise for that.

For now, the best solution is to depend on that commit hash: "sdk": "php@05cab0d6502599671a9da7d338011edd3a76129c"
I would rather not go back to using the direct link because that was clearly the root of the problem (and I might forget to update it next time as well!)

I will update it to remove the specific hash when 0.14 is released

sterile stratus
#

We don't want to inject tags into a full GitHub URL, we should only do magic things for the top level aliases/names

safe moon
late ruin
#

no worries @safe moon . i am actually glad that we found the root cause. 👍 .

#

i pushed a commit to use "php@main" - is that acceptable, if not how do we handle that (for current scenario as well as future updates).

#

having the SDK use released version is not going to work well if there are changes in SDK which are not released yet

sterile stratus
#

this means that if we make backwards incompat changes, old versions of the dev module won't build

safe moon
sterile stratus
#

sounds good!

safe moon