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

[sys] Fix Sys.putEnv() with null as variable value on all targets #10402

Merged
merged 4 commits into from
Oct 29, 2021

Conversation

tobil4sk
Copy link
Member

@tobil4sk tobil4sk commented Sep 24, 2021

Fixes #10395

Running Sys.putEnv() now works consistently across targets when null is passed in as the value parameter, in which case it deletes the environment variable.

Testing for this case has been added as well.

This PR depends on the following PRs in other repositories:

Depends on:

tobil4sk added a commit to tobil4sk/haxelib that referenced this pull request Oct 3, 2021
@RealyUniqueName RealyUniqueName added this to the Release 4.3 milestone Oct 6, 2021
@RealyUniqueName
Copy link
Member

CI changes don't fit this PR.

@tobil4sk
Copy link
Member Author

tobil4sk commented Oct 6, 2021

Dropped the other commits to leave only the ones needed for this PR.

Adding the sys tests for nodejs is necessary here so that we can test if the issue has been fixed there (but i can separate it into a different PR if needed).

@tobil4sk
Copy link
Member Author

Created a separate PR (#10437) for enabling the sys tests on nodejs. I have a rebased version of this branch ready to push once that one is merged.

@RealyUniqueName
Copy link
Member

Resolve conflicts please.

For cs, lua, php, and python

Make the v parameter nullable on neko, hl, cpp, and java
Add `eval.luv.Env.unsetEnv()` as well
@tobil4sk
Copy link
Member Author

Resolved

@tobil4sk tobil4sk changed the title [sys] Fix Sys.putEnv() with null as variable value on all targets (#10395) [sys] Fix Sys.putEnv() with null as variable value on all targets Oct 19, 2021
@RealyUniqueName
Copy link
Member

Some tests fail.

@tobil4sk
Copy link
Member Author

These tests are failing because hxcpp, neko and nodejs require target specific changes in their respective repositories. I made PRs for these changes which I listed in the first post. Here they are again to confirm:

Once these are merged and the tests are using the fixed versions they should all pass.

@tobil4sk
Copy link
Member Author

The neko tests are still failing because the updated neko version isn't being deployed. For the Windows cpp tests I'll have to debug those.

- Reorganise sys tests to allow for testing pre-defined environment
variables
@tobil4sk
Copy link
Member Author

Windows CPP tests should be fixed once HaxeFoundation/hxcpp/pull/973 is merged.

@tobil4sk
Copy link
Member Author

I opened an issue about neko CI failing to upload the new binaries: HaxeFoundation/neko/issues/235

Once that is fixed and the new neko builds can be downloaded properly, the remaining failing tests (which are just neko) should run successfullly.

tobil4sk added a commit to tobil4sk/haxelib that referenced this pull request Oct 25, 2021
tobil4sk added a commit to tobil4sk/haxelib that referenced this pull request Oct 25, 2021
tobil4sk added a commit to tobil4sk/haxelib that referenced this pull request Oct 25, 2021
@tobil4sk
Copy link
Member Author

I made a PR for neko that might fix the deployment issue: HaxeFoundation/neko#236.

Then, to ensure this bug is fixed (for neko) with the next haxe release what has to be done is:

@Simn
Copy link
Member

Simn commented Oct 27, 2021

I appreciate your resilience in dealing with this stuff...

@tobil4sk
Copy link
Member Author

Thanks! Looks like this is going to take quite a bit more resilience because there's no way of knowing currently if that fix worked since a new failure appeared out of nowhere in the Mac build...

@tobil4sk
Copy link
Member Author

Alright, I think neko's mac CI issue is resolved, I listed the PRs needed to fix it here: HaxeFoundation/neko#240

Then we'll be able to see if the deployment is working...

@Simn
Copy link
Member

Simn commented Oct 29, 2021

Something is now broken on Linux:

Uncaught exception - load.c(237) : Failed to load library : /home/runner/work/_temp/neko-2.3.1-linux64/std.ndll (/lib/x86_64-linux-gnu/libm.so.6: version `GLIBC_2.29' not found (required by /home/runner/work/_temp/neko-2.3.1-linux64/std.ndll))
Makefile:105: recipe for target 'haxelib' failed

@tobil4sk
Copy link
Member Author

Yes... This just doesn't end 👀 I think for now what's best is to temporarily download the old version of neko for CI, because this isn't going to be an issue only for this PR.

@tobil4sk
Copy link
Member Author

Seems like an issue caused by building neko in ubuntu 20.04 and haxe in 18.04 (probably due to c compiler toolkit differences). I created a PR to update it here #10457. Another option could be to use 18.04 to bulid neko and see if that fixes it? That might potentially be better for backwards compatibility, but haven't tested it.

@tobil4sk
Copy link
Member Author

I created a PR as well to build neko in an older version of ubuntu as well: HaxeFoundation/neko#241. Hopefully either solution should work.

@Simn
Copy link
Member

Simn commented Oct 29, 2021

You are the hero we need!

@tobil4sk
Copy link
Member Author

Finally all green!

@Simn
Copy link
Member

Simn commented Oct 29, 2021

Wohoo! Let's quickly merge it before it changes its mind.

@Simn Simn merged commit e5c120c into HaxeFoundation:development Oct 29, 2021
@tobil4sk tobil4sk deleted the delete-env branch October 29, 2021 15:01
@tobil4sk
Copy link
Member Author

Thanks! I'll move the stuff about updating the version of neko haxe is packaged with to a new issue.

@tobil4sk tobil4sk mentioned this pull request Apr 1, 2023
8 tasks
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.

No consistent way to unset environment variable
3 participants