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.

Sign up for GitHub

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

Jump to bottom

filesystem & binaryplatforms: remove undefined vars from exports #43631

Merged
merged 2 commits into from
Jan 3, 2022
Merged

filesystem & binaryplatforms: remove undefined vars from exports #43631

merged 2 commits into from
Jan 3, 2022

Conversation

timholy
Copy link
Sponsor Member

@timholy timholy commented Jan 2, 2022

These are never defined. They show up as noise during the ambiguity test.

@timholy
filesystem: remove undefined vars from exports
4443e14 
These are never defined. They show up as noise during the ambiguity test.
@DilumAluthge
Copy link
Member

Could we add a CI job that fails if there are undefined exports?

I know that Aqua.jl has a similar functionality for package tests. It would be nice to have something like that for Base and stdlibs.

@timholy timholy changed the title filesystem: remove undefined vars from exports filesystem & binaryplatforms: remove undefined vars from exports Jan 2, 2022
@timholy
Copy link
Sponsor Member Author

timholy commented Jan 2, 2022

We could add that to detect_ambiguity (although that doesn't specifically test exports, these simply get flagged as undefined names in Base). The one hitch is the handling of active_repl and active_repl_backend, which are defined as globals in base/client.jl: run_main_repl.

@DilumAluthge
Copy link
Member

We could add that to detect_ambiguity (although that doesn't specifically test exports, these simply get flagged as undefined names in Base).

That seems like a good idea.

The one hitch is the handling of active_repl and active_repl_backend, which are defined as globals in base/client.jl: run_main_repl.

Ideally, we would fix our "detect undefined names" tooling to correctly detect globals that are defined inside functions.

@KristofferC
Copy link
Sponsor Member

Ref #28693. Maybe the test stopped working.

@timholy
Copy link
Sponsor Member Author

timholy commented Jan 2, 2022

Should still work. None of these names were exported by Base, they were exported by internal modules.

@KristofferC
Copy link
Sponsor Member

Ah, I see.

@fredrikekre fredrikekre merged commit a3c2798 into master Jan 3, 2022
@fredrikekre fredrikekre deleted the teh/rm_undef_exports branch January 3, 2022 17:23
vtjnash
vtjnash reviewed Jan 7, 2022
View reviewed changes
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a breaking change. We can define this to have a value (0o000?), but cannot delete them now.

base/filesystem.jl
Comment on lines -48 to -51
JL_O_TEMPORARY,
JL_O_SHORT_LIVED,
JL_O_SEQUENTIAL,
JL_O_RANDOM,
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are defined, just perhaps not on your system: https://github.com/JuliaLang/julia/blob/master/src/file_constants.h

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, we should perhaps switch to the correct names for them (e.g. http:https://docs.libuv.org/en/v1.x/fs.html#c.UV_FS_O_SEQUENTIAL)

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave the choice to you, and just revert this change (#43817)

timholy added a commit that referenced this pull request Jan 14, 2022
@timholy
Revert "filesystem & binaryplatforms: remove undefined vars from expo…
f7740a1 
…rts (#43631)"

This reverts commit a3c2798.
@timholy timholy mentioned this pull request Jan 14, 2022
Merged
timholy added a commit that referenced this pull request Feb 15, 2022
@timholy
Enable suppression of warnings from ambiguity & unbound tests (#43817)
7cd11a6 
* Revert "filesystem & binaryplatforms: remove undefined vars from exports (#43631)"

This reverts commit a3c2798.

* Enable suppression of warnings in ambiguity & undefined tests
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request Feb 17, 2022
@timholy @antoine-levitt
Enable suppression of warnings from ambiguity & unbound tests (JuliaL…
2866d78 
…ang#43817)

* Revert "filesystem & binaryplatforms: remove undefined vars from exports (JuliaLang#43631)"

This reverts commit a3c2798.

* Enable suppression of warnings in ambiguity & undefined tests
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
@timholy @LilithHafner
filesystem & binaryplatforms: remove undefined vars from exports (Jul…
eade401 
…iaLang#43631)
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
@timholy @LilithHafner
Enable suppression of warnings from ambiguity & unbound tests (JuliaL…
9454725 
…ang#43817)

* Revert "filesystem & binaryplatforms: remove undefined vars from exports (JuliaLang#43631)"

This reverts commit a3c2798.

* Enable suppression of warnings in ambiguity & undefined tests
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
@timholy @LilithHafner
filesystem & binaryplatforms: remove undefined vars from exports (Jul…
a8dada5 
…iaLang#43631)
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
@timholy @LilithHafner
Enable suppression of warnings from ambiguity & unbound tests (JuliaL…
64b3b37 
…ang#43817)

* Revert "filesystem & binaryplatforms: remove undefined vars from exports (JuliaLang#43631)"

This reverts commit a3c2798.

* Enable suppression of warnings in ambiguity & undefined tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@timholy @DilumAluthge @KristofferC @vtjnash @fredrikekre