-
Notifications
You must be signed in to change notification settings - Fork 6
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
clean up version identifier #411
Comments
Also, ideally the logic for versions could be shared with yotta/build-server/etc. |
@ariel-phet recommended @jonathanolson take the lead and @samreid consult with respect to phet-io issues. |
@mattpen said that if build-server changes, it could also affect the website. |
Unassigning myself, @jonathanolson let me know if/when I can help out. |
Have we switched versioning schemes recently, as part of phetsims/chipper#560 or other issues? As this code will need to be used by perennial, presumably it will need to handle all versions of our version strings (presumably it will need to be somewhat lax with parsing in versions). |
My recollection is that we reached no consensus on version id syntax (in phetsims/chipper#560), agreed to live with the current warts imposed by PhET-iO (e.g. "1.0.0-phetiorc.1"), and decided to kick the can down the road until we work on new build tools/process to support PhET-iO. Before we can do that, we need to discuss high-level PhET-iO issues; in https://github.com/phetsims/phet-io/issues/1137#issuecomment-317140954, @kathy-phet said that discussion of those issues will resume "in early August". @ariel-phet said we should plan to spend time on build tools/process in October. |
@ariel-phet, I'd prefer to delay this work until we do the build tools/process stuff, as it hits build-server/chipper/maintenance-releases/yotta/sims/etc. Is that fine? |
@jonathanolson yes that should be fine. Related to phetsims/chipper#586 |
This is a key part of chipper:2.0, we should discuss fully when @pixelzoom and @kathy-phet are present as well. Things like phet-io studies, potentially other phet-io brands, will play into this. |
As part of chipper 2.0, version id schema was finalized and implement in phetsims/chipper#636 and phetsims/chipper#560, see SimVersion.js. Labeling for developer meeting to see what else needs to be done to address this issue. I'm interested in seeing this resolved mainly so that I can address #406 (disable "show answers" in production versions) for several of the sims that I'm responsible for. And we can't currently identify whether a sim is a production version at runtime. |
I'll consolidate sim-side logic into SimVersion, including the "is this running from production" checks. |
@jonathanolson it looks like this was completed, as SimVersion is looking pretty good. Please close if correct. I think that the rest will be covered by phetsims/perennial#111 |
It looks like there are still other cases of direct package.json usage (Sim.js), so I'm not sure the issue is completely handled. |
Scanned joist, and this is looking good. Closing. |
This issue was identified while working on #406.
PhET's version identifier has the following general problems:
• No complete specification of format and semantics (probably belongs in
SimVersion
).• Multiple incomplete specifications (
SimVersion
,getVersionForBrand
)• Multiple ways to get the version identifier (
require('JOIST/packageJSON')
,phet.joist.sim.version
,phet.chipper.version
,SimVersion
, others?)• Multiple representation of the version identifier (
{string}
,{SimVersion}
, others?)• Multiple places where the version identifier is being read from package.json (
Sim
,UpdateCheck
)• Brand names that don't match the name used in the metadata (e.g. "phet-io" and "phetio")
• No access by sims unless they violate visibility annotations (see phetsims/arithmetic#179, phetsims/molecule-shapes#147, phetsims/neuron#134)
• Sim code that does its own parsing of version identifiers (e.g phetsims/arithmetic#179)
PhET needs encapsulation of the version identifier (and its specification) in one representation, available to sims, without the need for sims to parse the identifier as a string.
@jonathanolson's intent may have been to address some of these issues by creating
SimVersion
. But he didn't replace any of the other representations, and SimVersion is used only byUpdateCheck
(the feature that checks for sim updates).Imo, this is high priority. Assigning to @ariel-phet to reprioritize and assign. Labeled for discussion at developer meeting.
The text was updated successfully, but these errors were encountered: