M-Labs/nix-scripts
9
0
Fork 9
Code Issues 6 Pull Requests Releases Wiki Activity

DAX: re-enable testing and build only on artiq-full #58

Merged
sb10q merged 4 commits from :feature/dax into master 2021-07-06 08:17:30 +08:00
Conversation 0 Commits 4 Files Changed 1 +9 -26
lriesebos commented 2021-06-30 02:15:05 +08:00
Contributor
Copy Link

@sb10q, DAX should preferably only test and build for artiq-full. Let me know if you find this an acceptable solution. If not, let me know what alternative approach you prefer.

I tried to test as much as possible of the changes, but I was not able to test all of them.

@sb10q, DAX should preferably only test and build for artiq-full. Let me know if you find this an acceptable solution. If not, let me know what alternative approach you prefer. I tried to test as much as possible of the changes, but I was not able to test all of them.
lriesebos added 3 commits 2021-06-30 02:15:05 +08:00
sb10q reviewed 2021-06-30 14:40:03 +08:00
hydra/artiq.json Outdated
@ -49,6 +49,7 @@
"nixpkgs": { "type": "git", "value": "git://github.com/NixOS/nixpkgs nixos-21.05", "emailresponsible": false },
"nixScripts": { "type": "git", "value": "https://git.m-labs.hk/M-Labs/nix-scripts.git", "emailresponsible": false },
"a6p": { "type": "boolean", "value": "true" },
"beta": { "type": "boolean", "value": "true" },
sb10q commented 2021-06-30 14:40:03 +08:00
Owner
Copy Link

Don't use beta. It will not be in beta forever. Instead create a new variable a7p. But why not have DAX available for ARTIQ-6?

Don't use ``beta``. It will not be in beta forever. Instead create a new variable ``a7p``. But why not have DAX available for ARTIQ-6?
lriesebos commented 2021-07-01 12:18:08 +08:00
Author
Contributor
Copy Link

DAX integrates very tightly with ARTIQ, and we only ensure compatibility with the current stable ARTIQ version, currently ARTIQ 6. Once ARTIQ 7 becomes the new stable version, we will make the jump to ARTIQ 7. So this moving beta flag should always signal the current ARTIQ beta version.

Currently I use the a6p and beta flags to make sure DAX only builds for the stable ARTIQ version, but instead we can also drop the beta flag in favor of a similar stable flag which is only true for the artiq-full channel. Then I do not rely on the a6p flag.

DAX integrates very tightly with ARTIQ, and we only ensure compatibility with the current stable ARTIQ version, currently ARTIQ 6. Once ARTIQ 7 becomes the new stable version, we will make the jump to ARTIQ 7. So this moving `beta` flag should always signal the current ARTIQ beta version. Currently I use the `a6p` and `beta` flags to make sure DAX only builds for the stable ARTIQ version, but instead we can also drop the `beta` flag in favor of a similar `stable` flag which is only `true` for the artiq-full channel. Then I do not rely on the `a6p` flag.
sb10q commented 2021-07-01 19:55:38 +08:00
Owner
Copy Link

In this case it makes more sense to test for ARTIQ major version = 6. And you can change it to 7 when you want to support that version.

This way, when 7 becomes stable and 6 becomes legacy, your package will keep working.

Note that this version check can usually be done without an additional Hydra boolean flag, such a flag was introduced only because of some edge cases related to #1.

In this case it makes more sense to test for ARTIQ major version = 6. And you can change it to 7 when you want to support that version. This way, when 7 becomes stable and 6 becomes legacy, your package will keep working. Note that this version check can usually be done without an additional Hydra boolean flag, such a flag was introduced only because of some edge cases related to https://git.m-labs.hk/M-Labs/nix-scripts/issues/1.
lriesebos commented 2021-07-03 04:02:35 +08:00
Author
Contributor
Copy Link

That's a better idea. The major version of DAX matches the major version of ARTIQ which it is designed to work with. I have updated this PR.

That's a better idea. The major version of DAX matches the major version of ARTIQ which it is designed to work with. I have updated this PR.
lriesebos marked this conversation as resolved
sb10q reviewed 2021-06-30 15:08:38 +08:00
artiq-full/extras.nix Outdated
@ -265,4 +257,0 @@
''
pytest
mypy
flake8
sb10q commented 2021-06-30 15:08:38 +08:00
Owner
Copy Link

Don't you still want mypy and flake8 to run?

Don't you still want mypy and flake8 to run?
lriesebos commented 2021-07-01 12:12:25 +08:00
Author
Contributor
Copy Link

Only Pytest is fine. We run all the extended tests + mypy + flake8 on our own CI infrastructure.

Only Pytest is fine. We run all the extended tests + mypy + flake8 on our [own CI infrastructure](https://gitlab.com/duke-artiq/dax/-/pipelines).
lriesebos marked this conversation as resolved
lriesebos force-pushed feature/dax from e236bdd629 to 3954805eb4 2021-07-03 03:59:37 +08:00 Compare
sb10q reviewed 2021-07-04 09:24:31 +08:00
artiq-full/extras.nix Outdated
@ -6,2 +6,2 @@
{ name, version, src, pythonOptions ? {}, condaOptions ? {}, withManual ? true}:
{
{ name, version, src, pythonOptions ? {}, condaOptions ? {}, enabled ? true, withManual ? true}:
pkgs.lib.optionalAttrs enabled ({
sb10q commented 2021-07-04 09:24:31 +08:00
Owner
Copy Link

Why not simply use pkgs.lib.optionalAttrs on dualPackage in extras.nix?

Why not simply use `` pkgs.lib.optionalAttrs`` on ``dualPackage`` in ``extras.nix``?
lriesebos commented 2021-07-06 05:47:19 +08:00
Author
Contributor
Copy Link

If I apply pkgs.lib.optionalAttrs on dualPackage, then the DAX version (which I use in the boolean expression) is not defined yet. That could be solved with a let statement, but I thought this might be a more attractive solution. If you still prefer applying pkgs.lib.optionalAttrs on dualPackage, let me know.

If I apply `pkgs.lib.optionalAttrs` on `dualPackage`, then the DAX version (which I use in the boolean expression) is not defined yet. That could be solved with a `let` statement, but I thought this might be a more attractive solution. If you still prefer applying `pkgs.lib.optionalAttrs` on `dualPackage`, let me know.
sb10q reviewed 2021-07-05 10:37:05 +08:00
artiq-full/extras.nix Outdated
@ -249,2 +243,3 @@
name = "dax";
version = "6.3";
version = "6.4";
enabled = (builtins.substring 0 1 version) == (builtins.substring 0 1 artiqVersion);
sb10q commented 2021-07-05 10:37:05 +08:00
Owner
Copy Link

This will break with version >=10. Use builtins.splitVersion.

This will break with version >=10. Use ``builtins.splitVersion``.
lriesebos commented 2021-07-06 05:42:47 +08:00
Author
Contributor
Copy Link

Still a lot of Nix to learn for me. Tnx for the tip!

Still a lot of Nix to learn for me. Tnx for the tip!
lriesebos marked this conversation as resolved
lriesebos force-pushed feature/dax from 3954805eb4 to ee4014f008 2021-07-06 05:41:29 +08:00 Compare
sb10q merged commit 162238b1f7 into master 2021-07-06 08:17:30 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/nix-scripts#58
No description provided.
Delete Branch ":feature/dax"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?