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

Should "custom" versions of libraries be allowed #526

Closed
tobil4sk opened this issue Oct 2, 2021 · 17 comments · Fixed by #510
Closed

Should "custom" versions of libraries be allowed #526

tobil4sk opened this issue Oct 2, 2021 · 17 comments · Fixed by #510

Comments

@tobil4sk
Copy link
Member

tobil4sk commented Oct 2, 2021

While working on my PR to rewrite the client code, I noticed that it is currently possible to create a "custom" version of a library and use it. I am wondering whether it's ok to remove this to make the code make more sense or whether it was ever meant to be an actual feature?

Steps to reproduce:

  • Copy an existing library version and rename it to custom, or anything:
haxelibrepo/
├─ libname/
│  ├─ 1.9.2/
│  ├─ custom/
│  ├─ .current
  • Run haxelib set libname custom
  • The library version is now set

Reasons why this shouldn't be allowed:

  • It might be better to have more control over which versions are set, and give an error if the user tries to use an invalid one
  • This version of the library might be from neither the haxelib database nor a git/vcs repository. Allowing for this would make it harder in the future to be more strict about tracking library versions.

So, I see this as a bug, but I just wanted to confirm.

@RealyUniqueName
Copy link
Member

I guess It's a side effect of haxelib git command implementation.

@tobil4sk
Copy link
Member Author

Yeah seems like it. Pretty easily fixed using abstracts. I assume it's fine to remove that in that case?

tobil4sk added a commit to tobil4sk/haxelib that referenced this issue Jan 6, 2022
- HaxeFoundation#364
- HaxeFoundation#526
- Ensure `list` doesn't break with invalid directories in the repository
- Ensure `list` doesn't show invalid versions
- Ensure --skip-dependencies doesn't break installing from haxelib.json
- Ensure --cwd works
- Ensure specifying versions with run, path, and libpath commands works
properly
- Don't show update message if vcs lib was already up to date
- Ensure order of output of `path` is correct
- Ensure remove can't break dev path set inside git/hg version
tobil4sk added a commit to tobil4sk/haxelib that referenced this issue Jan 9, 2022
- HaxeFoundation#364
- HaxeFoundation#526
- Ensure `list` doesn't break with invalid directories in the repository
- Ensure `list` doesn't show invalid versions
- Ensure --skip-dependencies doesn't break installing from haxelib.json
- Ensure --cwd works
- Ensure specifying versions with run, path, and libpath commands works
properly HaxeFoundation#249
- Don't show update message if vcs lib was already up to date
- Ensure order of output of `path` is correct
- Ensure remove can't break dev path set inside git/hg version
tobil4sk added a commit to tobil4sk/haxelib that referenced this issue Jan 9, 2022
- HaxeFoundation#364
- HaxeFoundation#526
- Ensure `list` doesn't break with invalid directories in the repository
- Ensure `list` doesn't show invalid versions
- Ensure --skip-dependencies doesn't break installing from haxelib.json
- Ensure --cwd works
- Ensure specifying versions with run, path, and libpath commands works
properly HaxeFoundation#249
- Don't show update message if vcs lib was already up to date
- Ensure order of output of `path` is correct
- Ensure remove can't break dev path set inside git/hg version
tobil4sk added a commit to tobil4sk/haxelib that referenced this issue Apr 4, 2022
- HaxeFoundation#364
- HaxeFoundation#526
- Ensure `list` doesn't break with invalid directories in the repository
- Ensure `list` doesn't show invalid versions
- Ensure --skip-dependencies doesn't break installing from haxelib.json
- Ensure --cwd works
- Ensure specifying versions with run, path, and libpath commands works
properly HaxeFoundation#249
- Don't show update message if vcs lib was already up to date
- Ensure order of output of `path` is correct
- Ensure remove can't break dev path set inside git/hg version
@Simn Simn closed this as completed in #510 Apr 6, 2022
Simn pushed a commit that referenced this issue Apr 6, 2022
- #364
- #526
- Ensure `list` doesn't break with invalid directories in the repository
- Ensure `list` doesn't show invalid versions
- Ensure --skip-dependencies doesn't break installing from haxelib.json
- Ensure --cwd works
- Ensure specifying versions with run, path, and libpath commands works
properly #249
- Don't show update message if vcs lib was already up to date
- Ensure order of output of `path` is correct
- Ensure remove can't break dev path set inside git/hg version
@ncannasse
Copy link
Member

Wow that just completely broke our build system :'(

@ncannasse
Copy link
Member

It was actually a feature, could this be revert ? I don't see an actual problem with allowing this as long as it doesn't happen in the common case.

@tobil4sk
Copy link
Member Author

This is going to be complicated to revert... the version values used are now abstracts with strict validation rules 😅. How exactly have you been making use of this?

@ncannasse
Copy link
Member

All of our libs are stored in a single haxelib repo and so all the directories are in lib/<libname>/<libname>/ directory with .current set to <libname>

@ncannasse
Copy link
Member

it's absolutely necessary for us to allow custom versions as we have our own system for managing libraries that use tags instead of versions since they are managed differently (no public release)

@tobil4sk
Copy link
Member Author

Is there any chance you would be able to move to using .dev paths that point to these library locations instead? I made a tool that automates management of dev paths like this for git submodules, and it could be generalised for your setup. That would prevent having to make these exceptions in haxelib.

But if not, which commands would have to support this? set and path?

@ncannasse
Copy link
Member

Users might also have .dev so we don't want these to overwrite the one in our repo.
We need that for compilation, so only for path.

@ncannasse
Copy link
Member

We're also getting the following:

Error: Warning: Repository requires reformatting. To reformat, run `haxelib fixrepo`.
Error: Current set version of heaps is invalid.

I guess these two are related ?

@tobil4sk
Copy link
Member Author

tobil4sk commented Apr 13, 2022

Users might also have .dev so we don't want these to overwrite the one in our repo.

Is this still an issue if you're using a local repository? I assume lib is configured as your haxelib repository? You could have your libraries still in the lib folder, but have a local repository in .haxelib with local dev paths configured to these libraries in lib. And if it's local it shouldn't replace any existing global configuration. The tool automates something similar to this.

The other warning is about fixing potential library name capitalisation issues, so it isn't related, however that command may have fail with the custom versions.

@ncannasse
Copy link
Member

ncannasse commented Apr 13, 2022

Is this still an issue if you're using a local repository? I assume lib is configured as your haxelib repository? You could have your libraries still in the lib folder, but have a local repository in .haxelib with local dev paths configured to these libraries in lib. And if it's local it shouldn't replace any existing global configuration. The tool automates something similar to this.

We don't use local repos, it's a bit more custom/complex than that :) if we can just get the haxelib path fixed wrt custom version that should be fine for us.

@tobil4sk
Copy link
Member Author

Alright, could you check if your builds work with 3a26569? It should work now, but I still need to add proper tests for it.

I do still think however that this is more of a bug, and it's abusing the way haxelib works to handle dependencies in a way that would be better done by a custom tool. Can we say that if all the stuff from here is implemented for easier custom library paths we can disallow these extra cases? ;)

tobil4sk added a commit that referenced this issue Apr 13, 2022
@tobil4sk
Copy link
Member Author

Just to clarify, haxelib path somelib now works as expected even if .current is set to a custom path. However, haxelib path somelib:custom doesn't.

Regarding the fixrepo warning, I tested it and it seems not to mind custom directories, so it should be fine. Just for reference, the command exists to solve #529.

@ncannasse
Copy link
Member

I'm getting the following:

C:\>haxelib path heaps
Warning: Repository requires reformatting. To reformat, run `haxelib fixrepo`.
C:/Projects/hxtools/heaps/
-D heaps=1.9.1
C:/Projects/shiroTools/haxe/lib/format/format/
-D format=3.5.0

I think the Warning will cause a problem for Haxe compiler and should never be displayed with haxelib path
Also, I failed to see exactly what kind of fix is required there ?

@ncannasse
Copy link
Member

Uhm ok, running haxelib fixrepo creates a .repoversion set to 1, then haxelib path works.
I'm curious about this however, why would this be needed ? Are we planning a format change in haxelib repositories ?

@tobil4sk
Copy link
Member Author

The warning is printed to stderr, while the compiler reads from stdout, so that should not be an issue.

The details are described here. The fix is required (mostly on Linux) in order to make library names completely case insensitive for all haxelib commands. To summarise, it turns all library directories to be lowercase and adds .name files for any library names which contain non lowercase characters.

Also, it is useful to show the warning there, because that way if haxelib path fails, the compiler will print this warning as well as the "library not found" error. In some cases, this is useful information, as the library may be inaccessible due to the capitalisation issues in old versions of haxelib.

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 a pull request may close this issue.

3 participants