Build scripts should use buildexpression types. #2774
Conversation
mitchell-as
force-pushed
the
mitchell/dx-1961
branch
from
893eeb0 to
c437fb6
Compare
Initially build scripts and build expressions were developed in parallel. Now they should use common types.
mitchell-as
force-pushed
the
mitchell/dx-1961
branch
from
c437fb6 to
7f69c13
Compare
|
Test failures are due to nightly failures from what I can tell. Logs seem to indicate success as well as local tests. |
|
@MDrakos ping. I'd like to land this by the end of the day today. |
| platforms=["12345", "67890"], | ||
| requirements=[{name="language/python"}] |
There was a problem hiding this comment.
Was this order messing with the tests? If so then we need to allow for arbitrary order of solve arguments.
There was a problem hiding this comment.
This is why I filed DX-2221 (Implement a canonical/stable sort for buildscript output.) It doesn't have anything to do with build expressions, just with the order that the build scripts are written in.
| In *In `parser:"'in' ':' @@"` | ||
| Let *Let `parser:"'let' ':' @@"` | ||
| In *In `parser:"'in' ':' @@"` | ||
| Expr *buildexpression.BuildExpression |
There was a problem hiding this comment.
It's not immediately clear to me why we're now embedding the expression within the script but this feels a bit off. Ideally, these two implementations would be separate as well as the code that does the translations.
There was a problem hiding this comment.
Originally I tried just that. However, I ended up writing a translator between participle-produced Go structs and build expression Go structs. That is very brittle and any changes to the build expression structs would need to be addressed in this package too -- it's one of the problems this ticket is trying to resolve.
One solution is to put participle tags into the build expressions and construct build scripts with that, but I figured build expressions should remain distinct from build scripts.
I found it's easiest to work directly with build expressions and JSON.
| func NewScriptFromBuildExpression(expr *buildexpression.BuildExpression) (*Script, error) { | ||
| return &Script{Expr: expr}, nil | ||
| } |
There was a problem hiding this comment.
Shouldn't this also create the Script? Is there a potential that a user calling a script method could get an error because the struct only has an expression assigned?
There was a problem hiding this comment.
Not yet. Any merges that need to take place will be done before writing to disk. That's how it was before, so I don't believe I've changed anything. There is no risk with this approach, as the participle fields are only used by other constructors.
|
I will follow up with PB about your question. Until I hear back, I've responded to your initial feedback. Thanks! Let me know if you think I need to change something or add documentation, etc. |
mitchell-as
force-pushed
the
mitchell/dx-1961
branch
from
3e921fc to
4ef1b95
Compare
Build script/build expression equality is suspect at this time.
| }, script) | ||
| } | ||
|
|
||
| func TestComplex(t *testing.T) { | ||
| // Re-enable when multiple solves are supported in DX-2238. | ||
| func NoTestComplex(t *testing.T) { |
There was a problem hiding this comment.
Sorry to be nitpicky but can we do this with a t.Skip() for now? It just makes it more explicit imo.
Initially build scripts and build expressions were developed in parallel. Now they should use common types.